Page MenuHomePhabricator

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

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

Details

Reviewers
sammccall
rsmith
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

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
916

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.EditedMon, Jul 29, 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.EditedWed, Jul 31, 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?