Page MenuHomePhabricator

[Sema] Don't skip function bodies with 'auto' without trailing return type
ClosedPublic

Authored by ilya-biryukov on Mar 14 2018, 9:17 AM.

Details

Summary

Skipping them was clearly not intentional. It's impossible to
guarantee correctness if the bodies are skipped.
Also adds a test case for r327504, now that it does not produce
invalid errors that made the test fail.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Mar 14 2018, 9:17 AM

Adding Richard as a reviewer, because this seems somewhat clever to me (so there may be a better change to make).

lib/Sema/SemaDecl.cpp
12611 ↗(On Diff #138388)

Please don't use auto here as the type is not spelled out explicitly. Also, the type can be const-qualified.

  • Replace auto with explicit type
ilya-biryukov marked an inline comment as done.Mar 27 2018, 7:31 AM

I hit that bug in practice a few days ago when playing around with a C++17 codebase. It makes completion totally unusable when functions with auto-deduced return type are used.
@rsmith, any chance you could take a look? Or maybe suggest someone else as a reviewer?

rwols added a subscriber: rwols.May 11 2018, 10:27 AM

Can we get this merged soon? I'm experiencing a lot of false positive diagnostics with this library at work.

Expounding on my concerns a bit -- I'm worried about all the other places calling isUndeducedType() and whether they're also at risk and thus a better fix is to change isUndeducedType() to pay attention to the language mode.

lib/Sema/SemaDecl.cpp
12611 ↗(On Diff #139926)

This can be marked const.

Thanks, I think this can be simplified a bit but this looks like the right fix.

lib/Sema/SemaDecl.cpp
12609–12610 ↗(On Diff #139926)

I don't think this comment captures the problem. Rather, I think the problem is that in a template we deduce the auto to a dependent type, so it's not considered "undeduced" at the point when we ask this question.

12612–12613 ↗(On Diff #139926)

Is there any need to check whether the type has been deduced here? It seems simpler (and equivalent) to turn off skipping for functions whose return type involves a deduced type regardless of whether deduction has been performed already (which it hasn't).

ilya-biryukov marked 2 inline comments as done.
  • Address review comments.
  • Added one more test.

Sorry for delays with this patch, I somehow missed the email notification about it.

Expounding on my concerns a bit -- I'm worried about all the other places calling isUndeducedType() and whether they're also at risk and thus a better fix is to change isUndeducedType() to pay attention to the language mode.

I thought semantics of isUndeducedType() is actually what's most callers (e.g. diagnostics) want. That is "the type is undeduced and not dependent". But the name can definitely bring some confusion, so there might be other places that misinterpreted it.

@rsmith, it would be great if you could take another look when you have time.

lib/Sema/SemaDecl.cpp
12609–12610 ↗(On Diff #139926)

Thanks! That definitely explains the problem in a proper manner.

12612–12613 ↗(On Diff #139926)

Thanks! I misinterpreted the deduced type's semantics. I thought it also appears in the following case:
auto foo() -> int {}
But it doesn't.

rsmith accepted this revision.May 29 2018, 1:17 PM
rsmith added inline comments.
test/CodeCompletion/skip-auto-funcs.cpp
9 ↗(On Diff #148835)

is skipped -> is not skipped?

18 ↗(On Diff #148835)

is skipped -> is not skipped?

This revision is now accepted and ready to land.May 29 2018, 1:17 PM
  • Fix the comments
ilya-biryukov marked 2 inline comments as done.May 30 2018, 5:54 AM
This revision was automatically updated to reflect the committed changes.
nik added a subscriber: nik.May 30 2018, 8:39 AM

I've stumbled about this bug too and was looking into it and then I saw the mail about this change being submitted :)

I've ended up with a slightly different change (https://dpaste.de/PSpM/raw , instead of FD->getReturnType()->getContainedDeducedType() I have FD->getReturnType()->getAs<AutoType>()). It looks like the tests from this change pass with my change and my tests pass with this change (consider to add decltype(auto) tests, too!). So I'm wondering whether there is a test case that fails for one change but not the other (if there is, the test case should probably be added), or if there are differences in the performance.

I'm not familiar enough with the code base to justify the getAs<AutoType>(), I've just observed that it made my tests pass.

In D44480#1116359, @nik wrote:

I've stumbled about this bug too and was looking into it and then I saw the mail about this change being submitted :)

I've ended up with a slightly different change (https://dpaste.de/PSpM/raw , instead of FD->getReturnType()->getContainedDeducedType() I have FD->getReturnType()->getAs<AutoType>()). It looks like the tests from this change pass with my change and my tests pass with this change (consider to add decltype(auto) tests, too!). So I'm wondering whether there is a test case that fails for one change but not the other (if there is, the test case should probably be added), or if there are differences in the performance.

I'm not familiar enough with the code base to justify the getAs<AutoType>(), I've just observed that it made my tests pass.

I'm sure there should be no significant differences in performance. Adding decltype(auto) to the test would certainly be useful.
W.r.t. correctness I think at this point checks for DeducedType and AutoType are equivalent. There is only one other inheritor of DeducedType, the DeducedTemplateSpecializationType, and it should not appear in the function return type IIUC.

When the concepts get into the standard and clangd implements them, this might change, as the comment of DeducedType suggests:

/// Common base class for placeholders for types that get replaced by
/// placeholder type deduction: C++11 auto, C++14 decltype(auto), C++17 deduced
/// class template types, and (eventually) constrained type names from the C++
/// Concepts TS.

I don't know the exact details of the latest Concepts proposal, but would assume usages of constrained template parameters from Concepts TS shouldn't block skipping of function bodies. In which case, your fix seems to do the right thing and this one doesn't.
@rsmith, WDYT? Should we check for AutoType instead?

Does getAs<AutoType>() work correctly with function returning auto&?

nik added a comment.May 30 2018, 11:53 PM

Does getAs<AutoType>() work correctly with function returning auto&?

No. For

class Foo {} foo;
auto& return_auto_ref() { return foo; }
auto r6 = return_auto_ref();

the "getAs<AutoType>()" version will skip the function body and generate an error message on use, but "FD->getReturnType()->getContainedDeducedType()" works fine (will not skip the body, as it should here). OK, so now I have a rough idea what the "Contained" was referring too :)

In D44480#1117230, @nik wrote:

Does getAs<AutoType>() work correctly with function returning auto&?

the "getAs<AutoType>()" version will skip the function body and generate an error message on use, but "FD->getReturnType()->getContainedDeducedType()" works fine (will not skip the body, as it should here). OK, so now I have a rough idea what the "Contained" was referring too :)

Yep, getContainedDeducedType() call is definitely necessary. Added the tests for discussed cases in rL333735.