This is an archive of the discontinued LLVM Phabricator instance.

Fix PR25627 - false positive diagnostics involving implicit-captures in dependent lambda expressions.
Needs RevisionPublic

Authored by faisalv on Dec 6 2020, 10:51 AM.

Details

Reviewers
rsmith
aaron.ballman
wchilders
BRevzin
Group Reviewers
Restricted Project
Summary

[4/11/22] I am updating this patch and syncing it up w the current working tree - hopefully will post something within a week or so - and address wyatts comments too.

This patch attempts to address the following bugs involving lambda-captures:

  1. /https://bugs.llvm.org/show_bug.cgi?id=25627 which emits a false positive diagnostic for the following code: `void f() { constexpr int I = 10; [](auto a) { return I; }; };`
    • this occurs because our current logic does not recognize that even though our expressions are not instantiation dependent, if that expression is being used to initialize some dependent entity, an lvalue-to-rvalue conversion might still be introduced during the instantiation, which might avert odr-use. (we do not do return type deduction until instantiation).
    • to address this, I pass in the destination type to ActOnFullExpr, if the expression might be used in such an initialization, and use a PotentialResultsVisitor to determine the potential results of the full-expression, and then use all that information to avoid such false positive diagnostics.
  2. it fixes any unnecessary odr-uses triggered in discarded-value expressions and thus some tests marked as FIXMES.
    • we simply make a call to CheckLValueToRValueConversionOperand() that rebuilds the relevant AST marking each potential result as non-odr-used for discarded value expressions even in the absence of an lvalue-to-rvalue conversion.
  3. I do have some questions, regarding the intended behavior (assuming a constexpr local-entity I in the enclosing scope):
[] (auto a) {
  if constexpr (false) {
    const int& i = I; <-- This is diagnosed, should it be - even though it is never instantiated?
  }
};

[](auto a) {
  if constexpr (sizeof(a) < 0) {
    const int &i = I; <-- This is diagnosed, should it be - even though it is never instantiated?
  }
}

struct X {
  constexpr static int s = 10;
  int i = 10;
};

void f() {
  constexpr X x;
  [] { 
    int i = x.s;  // <-- This is diagnosed, should it be - even though it does not really use 'x' since 's' is static
    int j = x.i;  // this does not odr-use x.
  }; 
}

Thank you!

Diff Detail

Event Timeline

faisalv requested review of this revision.Dec 6 2020, 10:51 AM
faisalv created this revision.
faisalv edited the summary of this revision. (Show Details)Dec 6 2020, 5:16 PM
faisalv edited the summary of this revision. (Show Details)
faisalv edited the summary of this revision. (Show Details)Dec 6 2020, 5:30 PM
faisalv edited the summary of this revision. (Show Details)Dec 6 2020, 6:18 PM
wchilders requested changes to this revision.Dec 15 2020, 12:11 PM

I'm still "chewing on this", I'm not sure I fully understand the problem well enough to give great "big picture" feedback. In any case, I've left a few comments where I had a potentially helpful thought :)

clang/lib/Sema/SemaExprCXX.cpp
7743

This might be better restated as:

if (isa<VarDecl>(E->getMemberDecl())) {
7754

This likely should restore the previous state to prevent preemptive clearing of the flag, i.e.

bool WasVisitingArraySubscriptBase = VisitingArraySubscriptBaseExpr;
VisitingArraySubscriptBaseExpr = true;
Visit(E->getBase());
VisitingArraySubscriptBaseExpr = WasVisitingArraySubscriptBase;

I haven't given sufficient thought as to whether that would be problematic in this case; but it seems worth bringing up.

7813

This should probably handle the error case, no?

7881

Could you elaborate on the changes made to this function -- if for no other reason than my curiosity :)? This seems to be largely the same checks, in a different order and some control flow paths that used to return false now potentially return true and vice versa.

8016

Is there a succinct way to rewrite this that might improve readability? i.e.

bool SuccinctName = !IsFullExprInstantiationDependent && !IsVarNeverAConstantExpression &&
        IsNonCaptureableLambda && IsInitializingAnEntityWithDependentType &&
        !IsReferenceRelatedAutoTypeDeduction;
if (SuccinctName) {

Alternatively a short comment; It's pretty hard (at least for me) to tell at a glance what this branch is for.

8037

Minor, but there's an extra space here the pre-merge didn't catch

This revision now requires changes to proceed.Dec 15 2020, 12:11 PM
jrheath added a subscriber: jrheath.Apr 7 2022, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2022, 8:25 AM
faisalv added a reviewer: Restricted Project.Apr 7 2022, 10:50 AM
faisalv edited the summary of this revision. (Show Details)Apr 11 2022, 7:58 AM
shafik added a subscriber: shafik.Jan 12 2023, 8:40 PM

Is this PR still workable or does it need a major rework and should be abandoned?

I'll try and look into it this weekend and have some sort of updated news
for you by monday?
Faisal Vali

I'll try and look into it this weekend and have some sort of updated news
for you by monday?
Faisal Vali

Awesome, feel free to add me as a reviewer. If we can get this fixed that would awesome.

@Fznamznon This might be of interest to you
@faisalv are you still working on this?

@Fznamznon This might be of interest to you
@faisalv are you still working on this?

Yeah - sorry - been a little distracted with other stuff.
I tried uploading an updated patch to phabricator - but i get an exception - do i need to figure out how to convert this review to a git pull request to move this fwd? Thanks!