Page MenuHomePhabricator

[Sema] Emit diagnostics for uncorrected delayed typos at the end of TU
ClosedPublic

Authored by ilya-biryukov on Jul 16 2019, 8:02 AM.

Details

Summary

Instead of asserting all typos are corrected in the sema destructor.

The sema destructor is not run in the common case of running the compiler
with the -disable-free cc1 flag (which is the default in the driver).

Having this assertion led to crashes in libclang and clangd, which are not
reproducible when running the compiler.

Asserting at the end of the TU could be an option, but finding all
missing typo correction cases is hard and having worse diagnostics instead
of a failing assertion is a better trade-off.

For more discussion on this, see:
https://lists.llvm.org/pipermail/cfe-dev/2019-July/062872.html

Diff Detail

Event Timeline

ilya-biryukov created this revision.Jul 16 2019, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 8:02 AM
Herald added a subscriber: kadircet. · View Herald Transcript

Fix a typo (xD)

rnk edited reviewers, added: rsmith; removed: rnk.Jul 16 2019, 3:38 PM
rnk added a subscriber: rnk.

I'm in favor of this as is. We should loop in and get confirmation from @rsmith though.

vsapsai added inline comments.
clang/test/SemaObjC/typo-correction-subscript.m
1

I think you can even remove -disable-free in this test. It was there to avoid the assertion in ~Sema (see D60848).

I tried to find a good place to emit unresolved typos earlier (to make sure CodeGen does not ever get TypoExpr), but couldn't find one.
Please let me know if there's some obvious place I'm missing.

Also open to suggestions for putting assertions on whether codegen is being called on TypoExprs.
From looking at the code, it seems that currently if the codegen eventually arrives at a TypoExpr, it will emit an error saying this expression is not yet supported.
There might be other failure modes (computing values of exprs containing TypoExpr, etc), but it feels like the current state should catch most of the potential bugs there.

This change seems to be an improvement over what we had before (clangd and libclang do not crash, no errors are lost) and it's very simple. So unless people object I would propose to land it even if it does not solve all of the problems around delayed exprs.

  • Remove -disable-free from the test, it is no longer required to workaround the crash

Big +1 from me, though I can't say I understand this well enough to substitute for Richard here.

clang/lib/Sema/Sema.cpp
940

If you like, DEBUG_WITH_TYPE("DelayedTypos", ...) would still let people who want to fix these see them. In practice, I suspect nobody actually wants to fix these, though :-(

I tried to find a good place to emit unresolved typos earlier (to make sure CodeGen does not ever get TypoExpr), but couldn't find one.
Please let me know if there's some obvious place I'm missing.

The original plan when we were first designing the feature was to emit these diagnostics when we pop an expression evaluation context. Maybe we could try that? If there's a reason to defer typo correction across such contexts, it might probably be rare enough that we can explicitly handle that and manually move the delayed typos to the surrounding context.

unless people object I would propose to land it even if it does not solve all of the problems around delayed exprs.

Sounds good to me.

@rsmith, I'll look into emitting the typos when we pop expression evaluation context, but do we expect this to cover all cases where TypoExprs are produced?
(conservatively assuming that the answer is "no") should we land this patch and also emit at the end of TU in addition to expression evaluation context?

rnk added a comment.Jul 18 2019, 1:27 PM

@rsmith, I'll look into emitting the typos when we pop expression evaluation context, but do we expect this to cover all cases where TypoExprs are produced?
(conservatively assuming that the answer is "no") should we land this patch and also emit at the end of TU in addition to expression evaluation context?

I was going to pose the question this way: suppose clang already diagnosed typos when leaving an expr evaluation context, when appropriate. Would it still make sense to relax this assertion to diagnose any remaining ones at end of TU? Are we confident that we can catch all the typos, always? I'm not confident that everything will be handled, so I think we should take this change as is.

In D64799#1592263, @rnk wrote:

@rsmith, I'll look into emitting the typos when we pop expression evaluation context, but do we expect this to cover all cases where TypoExprs are produced?
(conservatively assuming that the answer is "no") should we land this patch and also emit at the end of TU in addition to expression evaluation context?

I was going to pose the question this way: suppose clang already diagnosed typos when leaving an expr evaluation context, when appropriate. Would it still make sense to relax this assertion to diagnose any remaining ones at end of TU? Are we confident that we can catch all the typos, always? I'm not confident that everything will be handled, so I think we should take this change as is.

There may still be uncorrected typos left behind in the outermost ExprEvalContext (created by the Sema constructor). In principle we should be able to get rid of that and parse all expressions in an evaluation context created for that expression (and anywhere we can't do that is a bug because we'll be parsing an expression without specifying whether it's potentially-evaluated, etc), but in practice it looks like there are still at least a few places where we parse expressions with no expression evaluation context, so cleaning up the typos in ExprEvalContext[0] from ActOnEndOfTranslationUnitFragment seems reasonable to me too.

@rsmith, emitting the typos on pop expression evaluation context resulted in a bunch of failures when trying to transform the resulting errors, see a typical stacktrace below.
It seems we can actually pop expression evaluation contexts between producing and correcting a typo expression.

I don't see how to workaround this without digging further into typo correction and changing its design.
Would you be ok with landing the workaround as is? Alternatively, any ideas on how we can avoid this problem without extending the scope of the patch too much?

FAIL: Clang :: CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp (1595 of 15306)
******************** TEST 'Clang :: CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /usr/local/google/home/ibiryukov/projects/llvm/build-rel/bin/clang -cc1 -internal-isystem /usr/local/google/home/ibiryukov/projects/llvm/build-rel/lib/clang/10.0.0/include -nostdsysteminc -std=c++11 -verify /usr/local/google/home/ibiryukov/projects/llvm/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp
--
Exit Code: 134

Command Output (stderr):
--
clang: /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaLookup.cpp:5378: const Sema::TypoExprState &clang::Sema::getTypoExprState(clang::TypoExpr *) const: Assertion `Entry != DelayedTypos.end() && "Failed to get the state for a TypoExpr!"' failed.
Stack dump:
0.      Program arguments: /usr/local/google/home/ibiryukov/projects/llvm/build-rel/bin/clang -cc1 -internal-isystem /usr/local/google/home/ibiryukov/projects/llvm/build-rel/lib/clang/10.0.0/include -nostdsysteminc -std=c++11 -verify /usr/local/google/home/ibiryukov/projects/llvm/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp
1.      /usr/local/google/home/ibiryukov/projects/llvm/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp:23:26: current parser token ';'
 #0 0x00000000043e1ea4 PrintStackTrace /usr/local/google/home/ibiryukov/projects/llvm/llvm/lib/Support/Unix/Signals.inc:533:13
 #1 0x00000000043e1ea4 PrintStackTraceSignalHandler(void*) /usr/local/google/home/ibiryukov/projects/llvm/llvm/lib/Support/Unix/Signals.inc:593:0
 #2 0x00000000043dfa9e llvm::sys::RunSignalHandlers() /usr/local/google/home/ibiryukov/projects/llvm/llvm/lib/Support/Signals.cpp:69:18
 #3 0x00000000043e22b8 SignalHandler(int) /usr/local/google/home/ibiryukov/projects/llvm/llvm/lib/Support/Unix/Signals.inc:385:1
 #4 0x00007fedabb7d3a0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x123a0)
 #5 0x00007fedaac0bcfb raise (/lib/x86_64-linux-gnu/libc.so.6+0x36cfb)
 #6 0x00007fedaabf68ad abort (/lib/x86_64-linux-gnu/libc.so.6+0x218ad)
 #7 0x00007fedaabf677f (/lib/x86_64-linux-gnu/libc.so.6+0x2177f)
 #8 0x00007fedaac04542 (/lib/x86_64-linux-gnu/libc.so.6+0x2f542)
 #9 0x0000000005f4ba2e LookupBucketFor<const clang::TypoExpr *> /usr/local/google/home/ibiryukov/projects/llvm/llvm/include/llvm/ADT/DenseMap.h:618:5
#10 0x0000000005f4ba2e find /usr/local/google/home/ibiryukov/projects/llvm/llvm/include/llvm/ADT/DenseMap.h:184:0
#11 0x0000000005f4ba2e find /usr/local/google/home/ibiryukov/projects/llvm/llvm/include/llvm/ADT/MapVector.h:154:0
#12 0x0000000005f4ba2e clang::Sema::getTypoExprState(clang::TypoExpr*) const /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaLookup.cpp:5376:0
#13 0x0000000005e94695 TransformTypoExpr /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaExprCXX.cpp:7740:27
#14 0x0000000005e94695 clang::TreeTransform<(anonymous namespace)::TransformTypos>::TransformExpr(clang::Expr*) /usr/local/google/home/ibiryukov/projects/llvm/build-rel/tools/clang/include/clang/AST/StmtNodes.inc:1277:0
#15 0x0000000005e708ea hasErrorOccurred /usr/local/google/home/ibiryukov/projects/llvm/clang/include/clang/Sema/Sema.h:7864:38
#16 0x0000000005e708ea TryTransform /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaExprCXX.cpp:7651:0
#17 0x0000000005e708ea Transform /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaExprCXX.cpp:7686:0
#18 0x0000000005e708ea clang::Sema::CorrectDelayedTyposInExpr(clang::Expr*, clang::VarDecl*, llvm::function_ref<clang::ActionResult<clang::Expr*, true> (clang::Expr*)>) /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaExprCXX.cpp:7784:0
#19 0x0000000005c5deae isInvalid /usr/local/google/home/ibiryukov/projects/llvm/clang/include/clang/Sema/Ownership.h:208:37
#20 0x0000000005c5deae clang::Sema::AddInitializerToDecl(clang::Decl*, clang::Expr*, bool) /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaDecl.cpp:11236:0
#21 0x0000000005a22997 clang::Parser::ParseDeclarationAfterDeclaratorAndAttributes(clang::Declarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::ForRangeInit*) /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Parse/ParseDecl.cpp:2456:3
#22 0x0000000005a2093c clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::SourceLocation*, clang::Parser::ForRangeInit*) /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Parse/ParseDecl.cpp:2115:21
#23 0x00000000059eff85 clang::Parser::ParseDeclOrFunctionDefInternal(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Parse/Parser.cpp:1095:10
#24 0x00000000059ef9a5 clang::Parser::ParseDeclarationOrFunctionDefinition(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Parse/Parser.cpp:1111:12
#25 0x00000000059eea9a clang::Parser::ParseExternalDeclaration(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec*) /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Parse/Parser.cpp:931:12
#26 0x00000000059ecaa3 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, bool) /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Parse/Parser.cpp:682:10
#27 0x00000000059e8218 clang::ParseAST(clang::Sema&, bool, bool) /usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Parse/ParseAST.cpp:157:5

@rsmith, emitting the typos on pop expression evaluation context resulted in a bunch of failures when trying to transform the resulting errors, see a typical stacktrace below.
It seems we can actually pop expression evaluation contexts between producing and correcting a typo expression.

I think I see the problem: if we have expression E1 as a subexpression of E2, but E1 has its own expression evaluation context (eg, maybe it's in a sizeof expression or similar), we'll now diagnose E1's typos when we leave that context. That by itself seems fine, but then when we run typo correction for E2, we rediscover the typo from E1 and are surprised to find that the correction information has been discarded but the TypoExpr is still reachable in the AST. It seems like we could handle that by tracking the already-corrected TypoExprs and teaching TransformTypos to substitute in the known correction in this case rather than trying to pick a correction for itself.

Would you be ok with landing the workaround as is? Alternatively, any ideas on how we can avoid this problem without extending the scope of the patch too much?

As above, I think we can probably fix this without changing too much by tracking which typos we've already resolved rather than throwing away the information about them. Failing that, I can live with this landing as-is, but delaying these diagnostics to the end of the file is going to result in a bad user experience whenever it happens -- and I suspect it'll start happening significantly more than the assert currently fails, because we'll no longer have this assertion forcing people to call CorrectDelayedTyposInExpr when discarding an expression.

ilya-biryukov added a comment.EditedJul 29 2019, 4:24 PM

Thanks for the suggestion. Sounds reasonably simple, I'll try this and report back with the result.

One problem I don't know how to resolve yet is figuring out what to do with the diagnostic coming from the typo correction if the same error has already been reported.

In the above example, say we reported an error for E1 (without doing correction) and now we are attempting to run typo correction over E2, which contains E1 as a subexpression.

When we hit the typo inside E1, we have the following options:

  1. Correct a typo, emit another diagnostic.
  2. Correct a typo, do not emit diagnostic.
  3. Do not correct a typo.

(3) seems like a regression, making the feature not useful.
(2) means we do not notify users that typo correction happened.
(1) means we produce multiple diagnostics for the same error (identifier is unresolved).

Tried the suggested approach and ran into the issue described above.
Either we make the diagnostics less useful ('did you mean foo::bar?' turns into 'unresolved identifier bar'; without mentioning the correction) or we even stop providing some corrections in addition to that.

On the other hand, I agree that over time we will start emitting too many diagnostics at the end of TU if keep the patch as is. That is not a good way.
There should be a better option for emitting the uncorrected diagnostics closer to where they are produced, but I don't have a good idea on what it should be off the top of my head. Ideas warmly welcome.

ilya-biryukov added a comment.EditedJul 31 2019, 3:40 AM

An experiment with popping on expression evaluation context failed, I couldn't find a good way to fix the problems described above.
Typo correction can and will run after the evaluation context where expression created is popped and the diagnostic we produce depends on the results of typo correction.
Emitting diagnostics on some, but not all context pops could be an option, but classifying those seems to be hard, would require looking closely at every expr eval context push.

Failing that, I can live with this landing as-is, but delaying these diagnostics to the end of the file is going to result in a bad user experience whenever it happens -- and I suspect it'll start happening significantly more than the assert currently fails, because we'll no longer have this assertion forcing people to call CorrectDelayedTyposInExpr when discarding an expression.

In practice, this assertion is not even checked most of the time. As mentioned earlier, it only fires when running clang -cc1 is triggered explicitly without -disable-free (and driver passes -disable-free by default).
We ended up in a situation when this assertion was not firing for awhile; clangd and libclang crash because of this assertion and nobody else notices because clang won't crash and won't produce the errors either.

I'm happy to either emit or leave out those errors, but I think we should absolutely get rid of assertion in the Sema destructor that only fires in clang -cc1 and source-level tools.
Placing it at some other place that would help to discover all missing calls to CorrectTypos to fix typo correction seems ok. However, this change is aimed at unbreaking libclang and clangd and not fixing typo correction (it looks like a much harder problem).

@rsmith two options for this patch seem to be:

  • silently ignore the errors (behavior before this patch),
  • show them to the user at the end of TU (what happens in the current version of the patch). This can result in bad UX, but we won't drop any errors on the floor.

Which one do you think we should prefer?

@rsmith two options for this patch seem to be:

  • silently ignore the errors (behavior before this patch),
  • show them to the user at the end of TU (what happens in the current version of the patch). This can result in bad UX, but we won't drop any errors on the floor. Which one do you think we should prefer?

Let's take the second option. It seems better to diagnose an error late than not at all.

rsmith accepted this revision.Aug 29 2019, 4:00 PM

LGTM: while this isn't a solution to the root cause of the issues here, it puts us in a better situation than the status quo.

This revision is now accepted and ready to land.Aug 29 2019, 4:00 PM

For the record, there was another change regarding the delayed typos in clang::Sema::~Sema(): D62648 [Sema][Typo] Fix assertion failure for expressions with multiple typos.

For the record, there was another change regarding the delayed typos in clang::Sema::~Sema(): D62648 [Sema][Typo] Fix assertion failure for expressions with multiple typos.

Note that D62648 only makes fixes to the typo correction application (e.g. when CorrectDelayedTyposInExpr is called), in particular caused by the recursive nature of typo correction (correcting typos might introduce more typos!). The assertion failure can still be caused by failing to correct Typos (e.g not calling CorrectDelayedTyposInExpr for a given Expression).

This patch seems like a good idea as currently it only impacts clangd/libclang in practice. But here a few thoughts about potentially fixing the root cause here:

  • It looks as if when initially implemented typos were meant to be propagated up through the ExprEvalContext (see the commit description) as opposed to automatically clearing for every context, perhaps due to the issues described above. It's unclear what the intended behavior is though - when should CorrectDelayedTyposInExpr be called? When are we in a stable state to do typo correction?
  • From my limited understanding there is one main issue here: failure to call CorrectDelayedTyposInExpr for an expression. When fixing the typo correction handling mentioned above, this could happen during TreeTransform due to an Expression being created (along with a typo) and then being thrown away (due to an error). The problem here is that once it is discarded, it becomes unreachable. I'm assuming the proper handling here is to CorrectDelayedTyposInExpr more frequently, but I'm not quite sure.
    • Could Sema check if a TypoExpr becomes unreachable? If so, we can check for unreachable TypoExprs, diagnose them, and potentially if in DEBUG emit a warning (although we'd need to track where a TypoExpr was created to really understand why it was thrown away).

Tried the suggested approach and ran into the issue described above.
Either we make the diagnostics less useful ('did you mean foo::bar?' turns into 'unresolved identifier bar'; without mentioning the correction) or we even stop providing some corrections in addition to that.

On the other hand, I agree that over time we will start emitting too many diagnostics at the end of TU if keep the patch as is. That is not a good way.
There should be a better option for emitting the uncorrected diagnostics closer to where they are produced, but I don't have a good idea on what it should be off the top of my head. Ideas warmly welcome.

I might be wrong here, but I thought that diagnostics were delayed until typo correction has been completed on an expression. For example, you'll only get something like unresolved identifier bar instead of did you mean foo::bar? only when you call the DiagHandler with either a proper TypoCorrection or an empty one. If so, I'm not sure how you'd get into this case if you're calling CorrectDelayedTyposInExpr and tracking which typos have been resolved already.

Tried the suggested approach and ran into the issue described above.
Either we make the diagnostics less useful ('did you mean foo::bar?' turns into 'unresolved identifier bar'; without mentioning the correction) or we even stop providing some corrections in addition to that.

On the other hand, I agree that over time we will start emitting too many diagnostics at the end of TU if keep the patch as is. That is not a good way.
There should be a better option for emitting the uncorrected diagnostics closer to where they are produced, but I don't have a good idea on what it should be off the top of my head. Ideas warmly welcome.

I might be wrong here, but I thought that diagnostics were delayed until typo correction has been completed on an expression. For example, you'll only get something like unresolved identifier bar instead of did you mean foo::bar? only when you call the DiagHandler with either a proper TypoCorrection or an empty one. If so, I'm not sure how you'd get into this case if you're calling CorrectDelayedTyposInExpr and tracking which typos have been resolved already.

I couldn't find a place in the code that would make sure the typo expressions will not be corrected later.
Therefore, this approach led to worse diagnostics (unresolved identifier bar vs did you mean baz?) in some situations, i.e. when a correction would take place somewhere higher up the callstack, but we decided to emit the error before that.

In particular, I tried emitting uncorrected typo diagnostics when popping expression evaluation contexts, per Richard's suggestion.

ilya-biryukov added inline comments.Wed, Oct 9, 2:57 AM
clang/lib/Sema/Sema.cpp
940

I've left it out for now, not sure what's the best way to report those: crash? show a message in stderr? (probably a short message, indicating the number of uncorrected delayed typos in stderr is enough)
Also second the concerns that it's not very useful since no-one is actually looking at these.

This revision was automatically updated to reflect the committed changes.