r225389 - Handle OpaqueValueExprs more intelligently in the TransformTypos tree
Richard Smith
richard at metafoo.co.uk
Mon Jan 26 17:19:17 PST 2015
LGTM for trunk and branch.
On Mon, Jan 26, 2015 at 3:46 PM, Kaelyn Takata <rikka at google.com> wrote:
>
>
> 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
>> >> >>
>> >> >>
>> >> >
>> >
>> >
>
>
More information about the cfe-commits
mailing list