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

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:12367
+  case Type::Class:                                                            \
+    llvm_unreachable("Unexpected " Kind ": " #Class);
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283