mirror of
https://github.com/microsoft/PowerToys.git
synced 2026-05-18 05:05:25 +02:00
[CmdPal and Run Calculator] Fix issues with log functions and spaces (#47767)
## Summary of the Pull Request
The Calculator components in Command Palette and Run produced incorrect
results or errors for `log` and `ln` inputs where spaces exist between
the function name and the argument list. This is because input
validation allowed for those spaces, but this was not respected in the
log mapping code which transforms user input into a string for
consumption by the expression evaluator engine.
The result of this is discrepancy was that:
- Natural log would be called instead of log base 10 for `log (n)`.
- An error would be shown for `ln (n)`.
For example:
<img width="556" height="166" alt="image"
src="https://github.com/user-attachments/assets/d2110292-ca8f-4635-bdef-9fa4f2211deb"
/>
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
- [x] Closes: #47759
<!-- - [ ] Closes: #yyy (add separate lines for additional resolved
issues) -->
- [ ] **Communication:** I've discussed this with core contributors
already. If the work hasn't been agreed, this work might be rejected
- [x] **Tests:** Added/updated and all pass
- [ ] **Localization:** All end-user-facing strings can be localized
- [ ] **Dev docs:** Added/updated
- [ ] **New binaries:** Added on the required places
- [ ] [JSON for
signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json)
for new binaries
- [ ] [WXS for
installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs)
for new binaries and localization folder
- [ ] [YML for CI
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml)
for new test projects
- [ ] [YML for signed
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml)
- [ ] **Documentation updated:** If checked, please file a pull request
on [our docs
repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys)
and link it here: #xxx
<!-- Provide a more detailed description of the PR, other things fixed,
or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
The issue was with this code:
a650504640/src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.Calculator/CalculateEngine.cs (L56-L58)
This maps user input such as `log(10)` or `ln(10)` to the functions the
back-end expression evaluator understands. For Mages and ExprTK, this is
mapped to `log10(n)` for base 10 logs or `log(n)` for natural logs.
Unfortunately, the input validator regex allows for spaces between the
function name and the start of the argument list, and the string replace
does not. When spaces exist:
- For `log (n)` - the log10 match is missed and the expression is
interpreted as a natural log, producing incorrect results.
- For `ln (n)` - the string is passed as-is to the back-end. Neither
Mages nor ExprTK recognise it, so an error is returned.
The fix is to replace the string replacement code with two regexes.
These are both forgiving of spaces between the function name and
argument list:
For log base 10: `"log(?![0-9])\\s*\\("`
For natural log: `"ln\\s*\\("`
Both are culture-agnostic and have `IgnoreCase` set. I took the
opportunity to remove the "en-US" culture from the
`DivisionByZeroRegex`, as it is not required.
<!-- Describe how you validated the behavior. Add automated tests
wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Added unit tests which ensure the log and ln functions perform
identically with or without spaces. Confirmed all unit tests for Command
Palette and Run still pass.
Manually confirmed that `log` and `ln` now produce correct results in
both applications.
This commit is contained in:
Dave Rayment
@@ -64,6 +64,14 @@ public class ExtendedCalculatorParserTests : CommandPaletteUnitTestBase
|
||||
["log2(3)", 1.58496250072116M],
|
||||
["log10(3)", 0.47712125471966M],
|
||||
["ln(e)", 1M],
|
||||
|
||||
// Space between function name and '(' must produce the same result
|
||||
// (regression test for the log-mapping bug).
|
||||
["ln (3)", 1.09861228866811M],
|
||||
["log (3)", 0.47712125471966M],
|
||||
["log2 (3)", 1.58496250072116M],
|
||||
["log10 (3)", 0.47712125471966M],
|
||||
|
||||
["cosh(0)", 1M],
|
||||
["1*10^(-5)", 0.00001M],
|
||||
["1*10^(-15)", 0.0000000000000001M],
|
||||
|
||||
@@ -53,9 +53,11 @@ public static partial class CalculateEngine
|
||||
|
||||
// mages has quirky log representation
|
||||
// mage has log == ln vs log10
|
||||
input = input.
|
||||
Replace("log(", "log10(", true, CultureInfo.CurrentCulture).
|
||||
Replace("ln(", "log(", true, CultureInfo.CurrentCulture);
|
||||
// Use regex replacements so optional whitespace between the function name and
|
||||
// '(' is handled correctly - "log (100)" must map to log10 just like "log(100)"
|
||||
// does. The negative lookahead prevents "log10" / "log2" from being touched.
|
||||
input = LogRegex().Replace(input, "log10(");
|
||||
input = LnRegex().Replace(input, "log(");
|
||||
|
||||
input = CalculateHelper.FixHumanMultiplicationExpressions(input);
|
||||
|
||||
@@ -135,6 +137,15 @@ public static partial class CalculateEngine
|
||||
return rounded / 1.000000000000000000000000000000000m;
|
||||
}
|
||||
|
||||
[GeneratedRegex("\\/\\s*0(?!(?:[,\\.0-9]|[box]0*[1-9a-f]))", RegexOptions.IgnoreCase, "en-US")]
|
||||
[GeneratedRegex("\\/\\s*0(?!(?:[,\\.0-9]|[box]0*[1-9a-f]))", RegexOptions.IgnoreCase)]
|
||||
private static partial Regex DivisionByZeroRegex();
|
||||
|
||||
// Case-insensitive match for "log" not followed by a digit, then optional whitespace,
|
||||
// then '('. The negative lookahead protects "log2" and "log10". A new log variant
|
||||
// like "logb" must be handled explicitly.
|
||||
[GeneratedRegex("log(?![0-9])\\s*\\(", RegexOptions.IgnoreCase)]
|
||||
private static partial Regex LogRegex();
|
||||
|
||||
[GeneratedRegex("ln\\s*\\(", RegexOptions.IgnoreCase)]
|
||||
private static partial Regex LnRegex();
|
||||
}
|
||||
|
||||
@@ -76,6 +76,14 @@ namespace Microsoft.PowerToys.Run.Plugin.Calculator.UnitTests
|
||||
new object[] { "log2(3)", 1.58496250072116M },
|
||||
new object[] { "log10(3)", 0.47712125471966M },
|
||||
new object[] { "ln(e)", 1M },
|
||||
|
||||
// Space between function name and '(' must produce the same result
|
||||
// (regression test for the log-mapping bug).
|
||||
new object[] { "ln (3)", 1.09861228866810M },
|
||||
new object[] { "log (3)", 0.47712125471966M },
|
||||
new object[] { "log2 (3)", 1.58496250072116M },
|
||||
new object[] { "log10 (3)", 0.47712125471966M },
|
||||
|
||||
new object[] { "cosh(0)", 1M },
|
||||
};
|
||||
|
||||
|
||||
@@ -53,9 +53,11 @@ namespace Microsoft.PowerToys.Run.Plugin.Calculator
|
||||
|
||||
// mages has quirky log representation
|
||||
// mage has log == ln vs log10
|
||||
input = input.
|
||||
Replace("log(", "log10(", true, CultureInfo.CurrentCulture).
|
||||
Replace("ln(", "log(", true, CultureInfo.CurrentCulture);
|
||||
// Use regex replacements so optional whitespace between the function name and
|
||||
// '(' is handled correctly - "log (100)" must map to log10 just like "log(100)"
|
||||
// does. The negative lookahead prevents "log10" / "log2" from being touched.
|
||||
input = LogRegex().Replace(input, "log10(");
|
||||
input = LnRegex().Replace(input, "log(");
|
||||
|
||||
input = CalculateHelper.FixHumanMultiplicationExpressions(input);
|
||||
|
||||
@@ -125,7 +127,16 @@ namespace Microsoft.PowerToys.Run.Plugin.Calculator
|
||||
return result;
|
||||
}
|
||||
|
||||
[GeneratedRegex("\\/\\s*0(?!(?:[,\\.0-9]|[box]0*[1-9a-f]))", RegexOptions.IgnoreCase, "en-US")]
|
||||
[GeneratedRegex("\\/\\s*0(?!(?:[,\\.0-9]|[box]0*[1-9a-f]))", RegexOptions.IgnoreCase)]
|
||||
private static partial Regex DivisionByZeroRegex();
|
||||
|
||||
// Case-insensitive match for "log" not followed by a digit, then optional whitespace,
|
||||
// then '('. The negative lookahead protects "log2" and "log10". A new log variant
|
||||
// like "logb" must be handled explicitly.
|
||||
[GeneratedRegex("log(?![0-9])\\s*\\(", RegexOptions.IgnoreCase)]
|
||||
private static partial Regex LogRegex();
|
||||
|
||||
[GeneratedRegex("ln\\s*\\(", RegexOptions.IgnoreCase)]
|
||||
private static partial Regex LnRegex();
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user