hokein added inline comments.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4869
LookupResult::NotFoundInCurrentInstantiation) {
+ if (SS.isEmpty())
+ // bail out if we don't have a NNS, this could be happened during
----------------
sammccall wrote:
> two concerns here:
> - it's not clear to me whether it's best to recover from this failed
> invariant or make a change elsewhere to ensure it holds
> - IIUC we should only return true if an error has been emitted. We've got
> some suggestion that it already has, but no smoking gun - maybe there are
> cases we've missed. (The failure mode here is clang exits with an error
> status but prints no errors)
yeah, the current fix is not quite ideal.
thinking more about this, I think we should not recover if the arg is
`DeclRefExpr`, reasons:
- looks like most important cases are covered by `DependentScopeDeclRefExpr`
and `CXXDependentScopeMemberExpr` already (no failing tests, though the test
coverage is weak)
- if the arg is a `DeclRefExpr` (no typo correction), we'll never into this
branch - the lookup should find the responding ValueDecl, which is not a
TypeDecl
- if the arg is a `DeclRefExpr` (from typo correction), we might run into this
branch, but we're less certain, and it might violate the assumption (NSS must
be non-null). dropping it doesn't seem bad.
================
Comment at: clang/test/Parser/cxx-template-decl.cpp:296
+class UsingImpl {};
+class AddObservation {
+ using Using =
----------------
sammccall wrote:
> If I understand right, AddObservationFn gets typo-corrected to AddObservation
> (the function). When we go to check whether it's valid, we look up the name
> and get the type instead (injected-class-name). so we figure "missing
> typename" but actually everything's just totally confused.
>
> The "best" solution I guess would be not allowing typo-correction to the
> shadowed name.
> But I think the next best thing is just not to allow "insert typename and
> recover" if the expression we're inserting around already has errors - this
> seems like we have low confidence at this point.
>
> So does requiring !Arg.getAsExpr().containsErrors() in the check for
> inserting typename work?
> My hope is this would result in "undeclared identifier AddObservationFn, did
> you mean AddObservation" ... "template argument must be a type".
> (Which is "wrong" in the sense that AddObservation is a type, but that bug is
> in the typo-correction logic, not here)
> If I understand right, AddObservationFn gets typo-corrected to AddObservation
> (the function). When we go to check whether it's valid, we look up the name
> and get the type instead (injected-class-name). so we figure "missing
> typename" but actually everything's just totally confused.
Yes, exactly. The inconsistence between different decls resolved by typo
correction (function) and the name lookup (class) leads to the crash.
> The "best" solution I guess would be not allowing typo-correction to the
> shadowed name.
it might be possible, but I'm not sure this would fit the design of
typo-correction, or make it worse
- in particular, TypoCorrection core emits all *visible* decls for a typo (in
this case, both the function and the class)
- TypoCorrection API provides flexibility (e.g. `CorrectionCandidateCallback` )
to allow the client to do customization of all candidates, e.g. filtering,
ranking (in this case, the class decl gets dropped somehow).
so, yeah it seems reasonable to fix the typo correction client side, to make it
suggest a type decl (rather than a non-type decl) for this particular case.
Maybe a better way to fix the issue:
1. improve the typo correction (https://reviews.llvm.org/D83025)
2. make `SemaTemplate.cpp` robust, see my another comment
either 1 or 2 can fix this particular crash case (1 will improve the
diagnostic), but I think we need both -- as we can't guarantee there is no typo
correction running into that code path.
> But I think the next best thing is just not to allow "insert typename and
> recover" if the expression we're inserting around already has errors - this
> seems like we have low confidence at this point.
> So does requiring !Arg.getAsExpr().containsErrors() in the check for
> inserting typename work?
unfortunately, this is not possible at the moment -- the error-bit isn't
propagated from TypoExpr to the corrected Expr, this is because the
transformation of TypoExpr -> Expr is not straightforward,
a particular code path (in parsing a template argument):
1. we see an identifier token `AddObservationFn`, and we clarify (in
`Sema::ClassifyName`) it to determine it is a type, an expr, etc
2. during the classify section, we perform a name lookup, but don't find
anything. ok, let's try the typo correction.
3. we find a typo-corrected candidate (`AddObservation` function), so we
annotate the current token is non-type (by setting the function decl)
4. in the later parsing step, we build a DeclRefExpr which points to the
function decl
if we want to propagate the error-bit from typo-expr to the built DeclRefExpr,
we need to carry the error-bit through the above code path somehow, which seems
non-trivial.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82738/new/
https://reviews.llvm.org/D82738
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
