[PATCH] D111283: [clang] template / auto deduction deduces common sugar

Wed Sep 14 13:47:53 PDT 2022

aaronpuchert added inline comments.

================
Comment at: clang/lib/AST/ASTContext.cpp:12367
+  case Type::Class:                                                            \
+    llvm_unreachable("Unexpected " Kind ": " #Class);
+
----------------
mizvekov wrote:
> aaronpuchert wrote:
> > mizvekov wrote:
> > > davrec wrote:
> > > > mizvekov wrote:
> > > > > davrec wrote:
> > > > > > mizvekov wrote:
> > > > > > > davrec wrote:
> > > > > > > > Could we just return `X` here? Would that just default to the old behavior instead of crashing whenever unforeseen cases arise?  
> > > > > > > No, I think we should enforce the invariants and make sure we are handling everything that can be handled.
> > > > > > > 
> > > > > > > Classing `TemplateTypeParm`  as sugar-free was what was wrong and we missed this on the review.
> > > > > > There might always going to be a few rare corner cases vulnerable to this though, particularly as more types are added and the people adding them don't pay strict attention to how to incorporate them here, and don't write the requisite tests (which seem very difficult to foresee and produce).  When those cases arise we will be crashing even though we could produce a perfectly good program with the intended semantics; the only thing that would suffer for most users is slightly less clear diagnostic messages for those rare cases.  I think it would be better to let those cases gradually percolate to our attention via bug reports concerning those diagnostics, rather than inconveniencing the user and demanding immediate attention via crashes.  Or am I missing something?
> > > > > I could on the same argument remove all asserts here and just let the program not crash on unforeseen circumstances.
> > > > > 
> > > > > On the other hand, having these asserts here helps us catch bugs not only here, but in other parts of the code. For example uniquing / canonicalization bugs.
> > > > > 
> > > > > If someone changes the properties of a type so that the assumptions here are not valid anymore, it's helpful to have that pointed out soon.
> > > > > 
> > > > > Going for as an example this specific bug, if there werent those asserts / unreachables in place and we had weakened the validation here, it would take a very long time for us to figure out we were making the wrong assumption with regards to TemplateTypeParmType.
> > > > > 
> > > > > I'd rather figure that out sooner on CI / internal testing rather than have a bug created by a user two years from now.
> > > > Yes its nicer to developers to get stack traces and reproductions whenever something goes wrong, and crashing is a good way to get those, but users probably won't be so thrilled about it.  Especially given that the main selling point of this patch is that it makes diagnostics nicer for users: isn't it a bit absurd to crash whenever we can't guarantee our diagnostics will be perfect?
> > > > 
> > > > And again the real problem is future types not being properly incorporated here and properly tested: i.e. the worry is that this will be a continuing source of crashes, even if we handle all the present types properly.
> > > > 
> > > > How about we leave it as an unreachable for now, to help ensure all the present types are handled, but if months or years from now there continue to be crashes due to this, then just return X (while maybe write something to llvm::errs() to encourage users to report it), so we don't make the perfect the enemy of the good.
> > > It's not about crashing when it won't be perfect. We already do these kinds of things, see the FIXMEs around the TemplateArgument and NestedNameSpecifier, where we just return something worse than we wish to, just because we don't have time to implement it now.
> > > 
> > > These unreachables and asserts here are about testing / documenting our knowledge of the system and making it easier to find problems. These should be impossible to happen in correct code, and if they do happen because of mistakes, fixing them sooner is just going to save us resources.
> > > 
> > > `llvm::errs` suggestion I perceive as out of line with current practice in LLVM, we don't and have a system for producing diagnostics for results possibly affected by FIXME and TODO situations and the like, as far as I know, and I am not aware of any discussions in that direction. I expect a lot of complexity and noise if we went this way.
> > > And I think this would have even less chance of working out if we started to incorporate the reporting of violation of invariants into that.
> > > 
> > > I think even just using raw `llvm::errs` on clang would be incorrect per design, and could likely break users that parse our output.
> > > 
> > > I think if months and years from now, if someone stumbled upon an assert firing here and, instead of understanding what is happening and then fixing the code, just removed / weakened the assert, that would simply not be good and I hope a reviewer would stop that from happening :)
> > I tend to agree that an assertion is appropriate for this. But you could turn this into
> > ```
> > assert(false && "...");
> > return X;
> > ```
> > which would still assert, but fall back to something "reasonable" in builds without assertions. The issue with `llvm_unreachable` is that it translates to `__builtin_unreachable()` in builds without assertions, and the optimizer takes advantage of that quite heavily. That's why `llvm_unreachable` is better left to places where we're pretty sure they're unreachable unless something went horribly wrong, such as after switches that handle all enumeration values.
> I see, that is reasonable.
> 
> But grepping for `assert(false` in the code base, I see that they are pretty rare, accounting for a bit less than 0.5% in `clang/` and 0.3% in `llvm/`, compared to the total number of asserts. They occur fifty times less than plain llvm_unreachables. I don't remember ever seeing them in practice, so I was not aware this was something we would do.
> 
> I feel like in these cases where we are using llvm_unreachable in this patch, I am 100% sure they are really unreachable, unless something went horribly wrong. But I mean I could be completely wrong as well, I must admit, so maybe in those cases that is what went horribly wrong? (Me being wrong I mean)
It depends on how likely you think it is that someone inadvertently violates the invariant without noticing (even after running the tests). For internal invariants or things unlikely to miss in a test, `llvm_unreachable` should be fine. The less certain you are that mistakes would be hard to make or easy to find, the more preferable would be a fallback. If you think this was a one-off blunder and is unlikely to reoccur, you can also stick to `llvm_unreachable`. So this was just a suggestion, and since I merely skimmed over the code I'm not the best to judge.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111283/new/

https://reviews.llvm.org/D111283