[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

Sun Nov 4 09:40:23 PST 2018

NoQ added inline comments.

================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+  // Get the value of the expression to cast.
+  const auto ValueToCastOptional =
+      C.getSVal(CE->getSubExpr()).getAs<DefinedOrUnknownSVal>();
----------------
aaron.ballman wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > ZaMaZaN4iK wrote:
> > > > lebedev.ri wrote:
> > > > > ZaMaZaN4iK wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > ZaMaZaN4iK wrote:
> > > > > > > > lebedev.ri wrote:
> > > > > > > > > ZaMaZaN4iK wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > `const auto *`
> > > > > > > > > > Why do we need this change here? If I understand correctly, with `const auto*` we also need change initializer to `C.getSVal(CE->getSubExpr()).getAs<DefinedOrUnknownSVal>().getPointer()`. But I don't understand why we need this.
> > > > > > > > > Is `ValueToCastOptional` a pointer, a reference, or just an actual `DefinedOrUnknownSVal`? I can't tell.
> > > > > > > > > (sidenote: would be great to have a clang-tidy check for this.)
> > > > > > > > `ValueToCastOptional` is `llvm::Optional<DefinedOrUnknownSVal>`
> > > > > > > See, all my guesses were wrong. That is why it should not be `auto` at all.
> > > > > > I don't agree with you for this case. Honestly it's like a yet another holywar question. If we are talking only about this case - here you can see `getAs<DefinedOrUnknownSVal>` part of the expression. this means clearly (at least for me) that we get something like `DefinedOrUnknownSVal`. What we get? I just press hotkey in my favourite IDE/text editor and see that `getAs` returns `llvm::Optional<DefinedOrUnknownSVal>`. From my point of view it's clear enough here.
> > > > > > 
> > > > > > If we are talking more generally about question "When should we use `auto` at all? " - we can talk, but not here, I think :)
> > > > > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable comes to mind.
> > > > > > What we get? I just press hotkey in my favourite IDE/text editor and see that getAs returns llvm::Optional<DefinedOrUnknownSVal>
> > > > > Which hotkey do i need to press to see this here, in the phabricator?
> > > > > 
> > > > > This really shouldn't be `auto`, if you have to explain that in the variable's name, justify it in review comments.
> > > > Ok, didn't know about such LLVM coding standard. Of course, with this information I will fix using `auto` here. Thank you.
> > > Actually, I disagree. In the Static Analyzer we use `auto` if the return type is in the name of the expression, and `getAs` may fail, so it returns an `Optional`. In the case where a nullptr may be returned to signal failure, `auto *` is used, so I believe that `auto` is appropriate here.
> > But don't change it back now, it doesn't matter a whole lot :D
> > Actually, I disagree. In the Static Analyzer we use auto if the return type is in the name of the expression, and getAs may fail, so it returns an Optional. 
> 
> The static analyzer should be following the same coding standard as the rest of the project. This is new code, so it's not matching inappropriate styles from older code, so there's really no reason to not match the coding standard.
> 
> > In the case where a nullptr may be returned to signal failure, auto * is used, so I believe that auto is appropriate here.
> 
> I disagree that `auto` is appropriate here. The amount of confusion about the type demonstrates why.
I confirm the strong local tradition of preferring `auto` for optional `SVal`s in new code, and i believe it's well-justified from the overall coding guideline's point of view. Even if you guys didn't get it right from the start, the amount of Analyzer-specific experience required to understand that it's an optional is very minimal and people get used to it very quickly when they start actively working on the codebase.

On one hand, being a class that combines LLVM custom RTTI with pass-by-value semantics (i.e., it's like `QualType` when it comes to passing it around and like `Type` when it comes to casting), optionals are inevitable for representing `SVal` dynamic cast results.

On the other hand, `SVal` is one of the most common classes of objects in the Static Analyzer (like, maybe, `Stmt` in Clang; i think a lot of people who are interested in Static Analyzer more than in the rest of Clang learn about `SVal`s earlier than about `Stmt`s), and in particular `SVal` casts are extremely common (around 3-4 `SVal::getAs<T>()` casts per a path-sensitive checker, not counting `castAs`, ~2x more common than arithmetic operations over `SVal`s, only ~3x less common than all sorts of `dyn_cast` in all checkers, including path-insensitive checkers), so it's something you get used to really quickly.

Writing `Optional<DefinedOrUnknownSVal> DV = V.getAs<DefinedOrUnknownSVal>();` in a lot of checker callbacks is annoying for pretty much all checker developers from day 1. This clutters the most important parts of the checker's logic: transfer functions and the definition of the error state. Most bugs are in *that* part of the code, and those bugs are *not* due to using `auto` for casts. Not using `auto` almost doubles the amount of code you need to write to perform a simple run-time type check. With a more verbose variable name or a bit more code around it, it also causes a line break.

And it only provides information that most of the readers either already know ("`SVal` is a value-type, so it uses optionals"), or memorize immediately after encountering this pattern once on their first day, or barely even care (optionals are used like pointers anyway). Being finally allowed to replace it with just `auto` after migration to C++11 was divine.

So i'm still strongly in favor of keeping this pattern included in the list of //"places where the type is already obvious from the context"//. This is the top-1 source of annoying boilerplate in Static Analyzer, and it's as easy to remember as it is to learn that `dyn_cast<T>(S)` returns a pointer (or a reference? - you need to look up the definition of `S` to figure this out, unlike `V.getAs<T>()` that is always an optional for `SVal`s).

P.S. Though i admit that coming up with a less annoying API is also a good idea :)
P.P.S. I guess some sort of `Optional<auto>` would have made everybody happy. But it's not a thing unfortunately :(

https://reviews.llvm.org/D33672