Counting Bugs in Windows Calculator | Hacker News Reader

Counting Bugs in Windows Calculator

habr.com

> The analyzer has detected two identically implemented functions. As their names, Convert and ConvertBack, suggest, they were meant to do different things, but the developers should know better.

This is not a bug. Look how the `BooleanNegationConverter` is used:

    <RadioButton x:Name="fullKeypad"
     ...
     IsChecked="{x:Bind Model.IsBitFlipChecked, Converter={StaticResource BooleanNegationConverter}, Mode=TwoWay}"/>

    <RadioButton x:Name="bitFlip"
     ...
     IsChecked="{x:Bind Model.IsBitFlipChecked, Mode=TwoWay}"/>

Two radio buttons bound to the same boolean value, one with the negation converter. If `fullKeypad` is checked, `IsBitFlipChecked` should be set to false. If `IsBitFlipChecked` is set to true, `fullKeypad.IsChecked` should be set to false. The converter needs to do the same operation in both directions.

Agree it's not a bug as I assume it's an implementation of the IValueConverter interface: https://docs.microsoft.com/en-us/dotnet/api/system.windows.d...

However since the method bodies are identical, IMO one should have the full implementation and then the other should just call the first. Still not a bug, just a code smell.

Is this PVS-Studio blogspam? They're always so proud of being able to detect the same things gcc/clang -Wall -Wextra detects.

seems so: https://www.viva64.com/en/b/0615/

PVS-Studio vs Compilers:

Clang: https://www.viva64.com/en/b/0108/ , https://www.viva64.com/en/b/0155/ , https://www.viva64.com/en/b/0446/

GCC: https://www.viva64.com/en/b/0425/

Those seem like potential bugs in GCC/Clang - that doesn't say whether GCC compiled with GCC or Clang compiled with Clang would produce the same warnings. Does GCC compile cleanly with gcc -Wall -Wextra -Werror?

Do you have Clang tests which are more recent than 8 years ago? Or, especially, any which were validated and accepted upstream?

The last check was in 2016: https://www.viva64.com/en/b/0446/ . Yes, it was few years ago because we were not interested in writing the same articles again and again. There are a lot of other interesting projects in the world: https://www.viva64.com/en/inspections/ . I also want to note that you assume that Clang was worse than PVS-Studio long time ago but now it can catch up. To that, I will say that all these years we’ve been working on analyzer development :). By the way, here is where you can read about the way it works: "Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities" - https://www.viva64.com/en/b/0592/

More simply, I'm just curious about what, as a developer in 2019, would be the selling point for using it if you were on a project which was relatively clean using Clang's warnings and static analysis toolchain, flawfinder, etc.

In 'Suspicious comparison of real numbers':

isActive = ((ratio > threshold) || (ActiveIfEqual && (ratio == threshold)));

is called out as bad code by the blogger; this feels very much like a false positive, given that we're checking an inequality already; just effectively changing it from > to >=

A good example of not just blindly following the linter...

Yeah that whole section on suspicious comparison looks like false positives by someone who didn't even bother to look at the code or doesn't understand it. Right after there is also the flagging of strod(rounded string) = 0.0 which I'm pretty sure is correct. They are checking whether the rounded string is exactly zero. Automated tooling is really only useful if you actually know what you are doing and can properly triage the output. "Look I ran this tool and it spit out some things" isn't very helpful.

Still, the comparison ratio == threshold has very little chance to turn out true.

When you want to check whether A >= B (where A and B are numbers represented as floats), you would probably want to use the check A >= B - epsilon.

It only has little chance to turn out true if they're calculated in different ways and accumulate different error. If you're e.g. dividing two pairs of integers and comparing them, regular equality is likely to be fine.

A relevant question is what epsilon should be.

I disagree strongly. It looks obvious to me that they can be equal and that this is a special case. The comparison is done only after checking for A<B, and only if a boolean called "ActiveIfEqual" is true. I mean, I don't know how much more obvious they could have made it.

All edge cases have very little chance of being true, and I would hazard a guess that most of a developers time, and most code is spent on edge cases.

So I'm not sure you can argue against a piece of code on the basis of likelihood.

Yet another example of "suspicious" test code which intention is very clear:

    TEST_METHOD(TestSwitchAndReselectCurrentlyActiveValueDoesNothing)
    {
      // *snip*
      vm.Value2Active = true;
      // Establish base condition
      VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount);
      vm.Value2Active = true;
      VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount);
    }

> [...] The analyzer has detected two identical code fragments executing immediately one after the other. It looks like this code was written using the copy-paste technique and the programmer forgot to modify the copies.

Here Value2Active is a C++/CX property rigged to a PropertyChanged event [1] with a macro:

    public ref class UnitConverterViewModel sealed: public Windows::UI::Xaml::Data::INotifyPropertyChanged
    {
        // other members omitted
        OBSERVABLE_OBJECT();
        OBSERVABLE_PROPERTY_RW(bool, Value2Active);
    };

Can you be sure that this actually does not have a side effect? I bet not. Indeed, the intention of the original test is clear: a property, once set ("Switch") to a particular value, should not update other states even when set ("Reselect") to that same value---as it will really trigger a side effect! This idempotency guarantee is an important interface contract worth testing, no matter what a linter and clueless blog author says.

[1] https://github.com/Microsoft/calculator/blob/master/docs/App...

Wow, I can imagine how much fun it is to debug such properties.

It's interesting to see the transition from the old Proprietary Microsoft, to the new Open-Source Microsoft. Opening up code like this, which is unpolished and has bugs, is a sign of good will and transparency which I appreciate.

Although considering this application Calculator could be one of the most-used programs on Windows, and they left that many errors in. It's scary to think how many errors could be in other parts of the system...

> considering this application Calculator could be one of the most-used programs on Windows

I'd seriously doubt that. I'm sure browsers and word processors are used far more often, and that's not even including programs that run every time the computer boots!

Agreed. Calculators in general, yes. The Calculator app on my PC, not so much. I’d also assume people using calculators that much would buy a physical one, it’s much more convinient.

I have a calculator button on my keyboard right above the ten-key. When I push it it opens my calculator app or brings it to foreground if it's already open. I consider it extremely convenient since I find I need a calculator often, but not often enough to justify leaving one always on my desk. Also I can copy and paste answers/intermediary results easily and it does conversions automagically (e.g. "5 cups in ounces").

Physical calculator? You mean a phone right? or just Googling the equation? or saying okay google and asking the equation to your phone so you can just say the words billion and not type like 9 fucking zeroes or something?, or use desmos? or wolframalpha?

Physical calculators are for classrooms.

A phone is in almost all regards worse than using a calculator with actual buttons. Voice is even less efficient. The major reason to use a physical calculator at your desk is because it's a dedicated device that's always ready, and fast. Anecdotally, being able to use it efficiently one-handed also plays a role.

BTW, I think that python/ipython cli is a much better calculator. I stopped using other calculators long time ago. Although I understand that it's probably too complicated for ordinary user.

There is no task executed in Excel that I can't do better and/or faster in ipython.

- Pivot Tables

- Reach goal.

- Simple curve fitting

- Drawing charts in general (even with matplotlib categorical bat charts are a world of pain and then they tell you to learn Altair or something.

- Tables with conditional formatting

- ...

To be frank, I do the same with the GNU Octave CLI, which is IMO much more suited to evaluating mathematical expressions, and very short programs. Out-of the box, that is.

The right answer is probably to use the right tool for the job, be it xcas, octave or python. Microsoft's calculator is probably better suited for unit conversion, for instance, although units(1) would be a contender :)

> and they left that many errors in.

How many of those errors are visible to end users in a way that actually matters? A 'suspicious' floating point conversion in a display aspect ratio check? A memory leak in a program that's almost entirely interactively driven? I don't see anything else terribly huge on the list either.

Software errors have an impact to the user and a cost to fix. These don't seem like errors where the ROI on the fix was likely to be positive, so maybe it's reasonable they were left in.

You should also consider the errors that Microsoft _removed_ from the calculator before passing too much judgement on their software development process. After all, this is the same company that replaced IEEE754 floating point in calculator with an arbitrary precision library. (At considerably more expense than any of the bug fixes indicaetd by the attached article)

https://blogs.msdn.microsoft.com/oldnewthing/20040525-00/?p=...

I think all the code errors like this is why some people are scared to open source their code. While it’s a great learning experience some people just get overwhelmed with the possibility of wrongness so they don’t.

I’d go as far as saying that this kind of posts worsen the situation. In general, if you find bugs in open source code, it’s better to quietly file bugs to the author instead of writing a full post listing errors people made. It’s like your high school teacher calling you out in front of class for mistakes you made in a test. Not cool.

This post was an advertisement for static code analysis, not a shaming. It even mentions that these bugs will probably soon be fixed, which is good for Microsoft.

This post shows some false-positives and present them as bugs. It is mildly disingenuous for the casual observer.

> Suspicious function sequence

As far as I remember, and it has been a while (and is virtually undocumented), with ETW the convention is to not call the EndX method when an error occurs as EndX is basically synonymous with `return`. This might have been carried over.

Either way, I'm sure there are false positives, as always with static analysis.

This seems like a stolen article. The URL should be changed to the original blog post: https://www.viva64.com/en/b/0615/

It is the same author - SvyatoslavMC on habr.com and Svyatoslav Razmyslov on viva64.com. So it isn't a problem.

They usually repost their articles on Habr themselves in their corporate blog, linking to the source. It's a very popular Russian IT resource, so they do it to promote their articles, nothing is stolen, it's just reposted.

Thanks for the pointer, that was actually interesting. No one looks at most old code bases unless they are changing something in particular, that's how old bugs hang around forever. Commercial software companies of the world, open source your old crappy utilities and the world may improve them. Of course you don't do that because you are embarassed at their poor quality. Based on my own personal experience :-). You should have seen the source code of sybase sql server.

This isn't an old code base though. It's the new UWP calculator that was introduced in Windows 10 (or was it 8?).

This is the current codebase but it goes back at least 20 years in places. If you look at the core engine you'll see plenty of comments dating back to 1995:

https://github.com/Microsoft/calculator/search?q=1995&unscop...

Calculator is interesting in that there is a physical button on (many) keyboards to open it. Ironically enough, I encountered a bug with the Windows 10 calculator that caused it to crash upon opening: the window would draw, then a second later vanish.

Turned out that it was due to something profile related: re-registering the Windows App Store fixed the issue. Yes, that's right, a dependency on the app store can prevent you from running Calculator.

There's another article on calculators' code check. This time is Qalculate - a multi-purpose cross-platform desktop calculator. https://habr.com/en/company/pvs-studio/blog/443656/

It’s interesting that most of these would have been avoided by using a safer language than C++.

Rust Evangelism Strike Force

Judging by the downvotes it's more like the C++ Evangelism Strike Force.

Not even Rust necessarily. I mean, this could have been avoided with any of the new age languages with a focus on safety.

Calculator has been around since ninteen-eighty-something-or-other[0] and this seems to be a revamp of the base code, with a XAML (read: WPF) wrapper[1].

Aside from the obvious answer of them using C++ based upon "using something you know and that works", why shouldn't they have used C++? (Genuinely wanting to know/understand.)

It would seem like a monolithic task to rearchitecture the entire app versus just dropping the code-behind into a new UI wrapper, yeah? I'm not excusing it, to be sure, just explaining the potential ROI perspective they might've had when they stuck with C++.

[0] - https://en.wikipedia.org/wiki/Windows_Calculator#History

[1] - https://en.wikipedia.org/wiki/Windows_Calculator#Windows_10

The code base has changed quite a bit since 198x:

- https://blogs.msdn.microsoft.com/oldnewthing/20040525-00/?p=...

- https://blogs.msdn.microsoft.com/oldnewthing/20160628-00/?p=...

- https://blogs.msdn.microsoft.com/oldnewthing/20180704-00/?p=...

This might also explain the choice of C++. Rather than rewriting their calculation engine in another language (it works, so a rewrite is rarely a good idea), UWP (not WPF; XAML is merely an XML dialect for object graphs and works pretty much anywhere if you want it to) allows you to write the UI in C++ (C++/CX to be precise) as well, and the UI code is for the most part not that complicated that C++ is a major impediment. That, and of course, most developers for Windows applications at Microsoft are familiar and well-versed in C++. Rewriting it in JavaScript or C# may take longer.

>Rewriting it in JavaScript or C# may take longer.

This is very true. If we look at Exchange, as an example, most of it was ported to .NET between E12 (2007) and E14 (2010) but Information Store wasn't able to be fully ported to C# until E15 (2013)[0,1].

[0] - https://docs.microsoft.com/en-us/exchange/managed-store-exch...

[1] - https://en.wikipedia.org/wiki/History_of_Microsoft_Exchange_...

It will be interesting to see how soon they will convert it to C++/WinRT because C++/CX will be retired at some point.

The first step toward full C++/WinRT support within Visual Studio and the eventual retirement of C++/CX happened today with the availability of build 17025 of the Windows SDK Insider Preview.

https://moderncpp.com/2017/11/01/cppwinrt-in-the-windows-sdk...

why shouldn't they have used C++

I didn't say they shouldn't, I just thought it was interesting to see.

Rust will always be niche. It won't win to C/C++. Sorry mate.

I didn't say anything about Rust, in fact I have never used it.

Yeah, nobody is talking about this but I think you're right on the money.

Reply on news.ycombinator.com