r225389 - Handle OpaqueValueExprs more intelligently in the TransformTypos tree
Kaelyn Takata
rikka at google.com
Mon Jan 26 15:46:30 PST 2015
On Tue, Jan 20, 2015 at 5:05 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
> On Tue, Jan 20, 2015 at 4:33 PM, Kaelyn Takata <rikka at google.com> wrote:
> >
> >
> > On Tue, Jan 20, 2015 at 3:26 PM, Richard Smith <richard at metafoo.co.uk>
> > wrote:
> >>
> >> On Tue, Jan 20, 2015 at 1:28 PM, Kaelyn Takata <rikka at google.com>
> wrote:
> >> >
> >> >
> >> > On Wed, Jan 14, 2015 at 7:36 PM, Richard Smith <richard at metafoo.co.uk
> >
> >> > wrote:
> >> >>
> >> >> On Wed, Jan 7, 2015 at 1:16 PM, Kaelyn Takata <rikka at google.com>
> wrote:
> >> >>>
> >> >>> Author: rikka
> >> >>> Date: Wed Jan 7 15:16:39 2015
> >> >>> New Revision: 225389
> >> >>>
> >> >>> URL: http://llvm.org/viewvc/llvm-project?rev=225389&view=rev
> >> >>> Log:
> >> >>> Handle OpaqueValueExprs more intelligently in the TransformTypos
> tree
> >> >>> transform.
> >> >>>
> >> >>> Also diagnose typos in the initializer of an invalid C++
> declaration.
> >> >>> Both issues were hit using the same line of test code, depending on
> >> >>> whether the code was treated as C or C++.
> >> >>>
> >> >>> Fixes PR22092.
> >> >>>
> >> >>> Modified:
> >> >>> cfe/trunk/lib/Sema/SemaDecl.cpp
> >> >>> cfe/trunk/lib/Sema/SemaExprCXX.cpp
> >> >>> cfe/trunk/test/Sema/typo-correction.c
> >> >>> cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp
> >> >>>
> >> >>> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> >> >>> URL:
> >> >>>
> >> >>>
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=225389&r1=225388&r2=225389&view=diff
> >> >>>
> >> >>>
> >> >>>
> ==============================================================================
> >> >>> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> >> >>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Wed Jan 7 15:16:39 2015
> >> >>> @@ -8574,8 +8574,10 @@ void Sema::AddInitializerToDecl(Decl *Re
> >> >>> bool DirectInit, bool
> >> >>> TypeMayContainAuto) {
> >> >>> // If there is no declaration, there was an error parsing it.
> Just
> >> >>> ignore
> >> >>> // the initializer.
> >> >>> - if (!RealDecl || RealDecl->isInvalidDecl())
> >> >>> + if (!RealDecl || RealDecl->isInvalidDecl()) {
> >> >>> + CorrectDelayedTyposInExpr(Init);
> >> >>> return;
> >> >>> + }
> >> >>>:61
> >> >>> if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(RealDecl)) {
> >> >>> // With declarators parsed the way they are, the parser cannot
> >> >>>
> >> >>> Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp
> >> >>> URL:
> >> >>>
> >> >>>
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=225389&r1=225388&r2=225389&view=diff
> >> >>>
> >> >>>
> >> >>>
> ==============================================================================
> >> >>> --- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)
> >> >>> +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Wed Jan 7 15:16:39 2015
> >> >>> @@ -6141,6 +6141,12 @@ public:
> >> >>>
> >> >>> ExprResult TransformLambdaExpr(LambdaExpr *E) { return Owned(E);
> }
> >> >>>
> >> >>> + ExprResult TransformOpaqueValueExpr(OpaqueValueExpr *E) {
> >> >>> + if (Expr *SE = E->getSourceExpr())
> >> >>> + return TransformExpr(SE);
> >> >>> + return BaseTransform::TransformOpaqueValueExpr(E);
> >> >>> + }
> >> >>
> >> >>
> >> >> Hmm, can you instead handle this by overriding TreeTransform's
> >> >> AlreadyTransformed:
> >> >>
> >> >> bool AlreadyTransformed(QualType T) {
> >> >> return T.isNull() || T->isInstantiationDependentType();
> >> >> }
> >> >>
> >> >> (Since we treat a TypoExpr as being dependent, it must lead to an
> >> >> instantiation-dependent type.)
> >> >
> >> >
> >> > That fixes the TreeTransform assertion reported in PR22092, but it
> isn't
> >> > sufficient for the TypoExpr nested inside of the OpaqueValueExpr to be
> >> > seen
> >> > as the default TransformOpaqueValueExpr just returns the original
> >> > OpaqueValueExpr if the assertion doesn't fail. So instead the test
> case
> >> > added to typo-correction.c fails in a different way:
> >> >
> >> > error: 'error' diagnostics expected but not seen:
> >> > File ~/llvm/tools/clang/test/Sema/typo-correction.c Line 13: use of
> >> > undeclared identifier 'b'
> >> > error: 'error' diagnostics seen but not expected:
> >> > (frontend): used type '<dependent type>' where arithmetic, pointer,
> or
> >> > vector type is required
> >> > clang: ../tools/clang/lib/Sema/Sema.cpp:249: clang::Sema::~Sema():
> >> > Assertion
> >> > `DelayedTypos.empty() && "Uncorrected typos!"' failed.
> >> > [followed by the stack trace for the failed assertion]
> >> >
> >> > Overriding the definition of AlreadyTransformed just for to get an
> >> > assertion
> >> > in one Tranform*Expr method to pass also feels a bit... heavy-handed
> to
> >> > me.
> >>
> >> The idea is that the OpaqueValueExpr is "owned" by some other AST
> >> node, and transforming that other AST node should cause the source
> >> expr of the OVE to be transformed and should recreate the OVE itself.
> >> So the TreeTransform should *never* see an OVE that needs
> >> transformation (actually, it should probably never see one at all...).
> >> In any case, it can't know how to transform one, because that's a
> >> non-local property and depends on the owner of the OVE.
> >>
> >> For a BinaryConditionalOperator, we transform the "common" expr (not
> >> an OVE) and the RHS expr (also not an OVE). I ran your patch against
> >> this testcase:
> >>
> >> int foobar; int f() { return goobar ?: 1; }
> >>
> >> ... and your TransformOpaqueValueExpr function was not called. So I
> >> think that part is entirely a red herring.
> >>
> >> In your testing, did you revert just the TransformOpaqueValueExpr part
> >> of this patch or also the change to SemaDecl (which I think was the
> >> real fix here)?
> >
> >
> > In my testing I only replaced the TransformOpaqueValueExpr with your
> > suggested AlreadyTransformed. Try testing the example added to
> > test/Sema/typo-correction.c, as this problem seems to be specific to the
> C
> > code path (the change to SemaDecl fixes the same example [which is pulled
> > from the PR] when it is treated as C++ code). Having "a = b ? : 0" at the
> > top level of a file causes the TreeTransform to see an OpaqueValueExpr
> when
> > the file is parsed as C code but not when it is parsed as C++ code.
>
> For
>
> int foobar; a = goobar ?: 4;
>
> I get
>
> <stdin>:1:13: warning: type specifier missing, defaults to 'int'
> [-Wimplicit-int]
> int foobar; a = goobar ?: 4;
> ^
> <stdin>:1:17: error: use of undeclared identifier 'goobar'; did you
> mean 'foobar'?
> int foobar; a = goobar ?: 4;
> ^~~~~~
> foobar
> <stdin>:1:5: note: 'foobar' declared here
> int foobar; a = goobar ?: 4;
> ^
> <stdin>:1:24: error: incompatible operand types ('<dependent type>' and
> 'int')
> int foobar; a = goobar ?: 4;
> ^ ~
>
> ... which suggests our error recovery isn't working properly here.
>
> Digging into this further, the issue is that the wrong thing is
> happening at the start of CheckConditionalOperands:
>
> if (!getLangOpts().CPlusPlus) {
> // C cannot handle TypoExpr nodes on either side of a binop because it
> // doesn't handle dependent types properly, so make sure any TypoExprs
> have
> // been dealt with before checking the operands.
> ExprResult CondResult = CorrectDelayedTyposInExpr(Cond);
> if (!CondResult.isUsable()) return QualType();
> Cond = CondResult;
> }
>
> This is not correct for a BinaryConditionalOperator; typo-correction
> should be applied to the common expression, not to the condition. The
> right thing to do would be to move this code up into
> ActOnConditionalOp (which should be the only caller of
> CheckConditionalOperands in C mode), to before we create the
> OpaqueValueExprs.
>
Ah, I see! Thanks for pointing me back in the right direction. Attached is
a patch that drops the custom TransformOpaqueValueExpr and hoists the typo
correction of the conditional to ActOnConditionalOp before the check for a
binary conditional operator. If it looks good to you, I'll submit it and
propose the commit for the 3.6 branch.
>
>
>> >>> +
> >> >>> ExprResult Transform(Expr *E) {
> >> >>> ExprResult Res;
> >> >>> while (true) {
> >> >>>
> >> >>> Modified: cfe/trunk/test/Sema/typo-correction.c
> >> >>> URL:
> >> >>>
> >> >>>
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/typo-correction.c?rev=225389&r1=225388&r2=225389&view=diff
> >> >>>
> >> >>>
> >> >>>
> ==============================================================================
> >> >>> --- cfe/trunk/test/Sema/typo-correction.c (original)
> >> >>> +++ cfe/trunk/test/Sema/typo-correction.c Wed Jan 7 15:16:39 2015
> >> >>> @@ -9,3 +9,6 @@ void PR21656() {
> >> >>> float x;
> >> >>> x = (float)arst; // expected-error-re {{use of undeclared
> >> >>> identifier
> >> >>> 'arst'{{$}}}}
> >> >>> }
> >> >>> +
> >> >>> +a = b ? : 0; // expected-warning {{type specifier missing,
> defaults
> >> >>> to
> >> >>> 'int'}} \
> >> >>> + // expected-error {{use of undeclared identifier
> 'b'}}
> >> >>>
> >> >>> Modified: cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp
> >> >>> URL:
> >> >>>
> >> >>>
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp?rev=225389&r1=225388&r2=225389&view=diff
> >> >>>
> >> >>>
> >> >>>
> ==============================================================================
> >> >>> --- cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp (original)
> >> >>> +++ cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp Wed Jan 7
> >> >>> 15:16:39 2015
> >> >>> @@ -152,3 +152,8 @@ namespace PR21947 {
> >> >>> int blue; // expected-note {{'blue' declared here}}
> >> >>> __typeof blur y; // expected-error {{use of undeclared identifier
> >> >>> 'blur'; did you mean 'blue'?}}
> >> >>> }
> >> >>> +
> >> >>> +namespace PR22092 {
> >> >>> +a = b ? : 0; // expected-error {{C++ requires a type specifier for
> >> >>> all
> >> >>> declarations}} \
> >> >>> + // expected-error-re {{use of undeclared identifier
> >> >>> 'b'{{$}}}}
> >> >>> +}
> >> >>>
> >> >>>
> >> >>> _______________________________________________
> >> >>> cfe-commits mailing list
> >> >>> cfe-commits at cs.uiuc.edu
> >> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >> >>
> >> >>
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150126/b930999e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tc-binary-conditionals-proper-fix.patch
Type: text/x-patch
Size: 3051 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150126/b930999e/attachment.bin>
More information about the cfe-commits
mailing list