Page MenuHomePhabricator

[clang] Show error when a local variables is passed as default template parameter
Needs ReviewPublic

Authored by massberg on Dec 6 2022, 12:55 AM.

Details

Reviewers
ilya-biryukov
Summary

I haven't found an existing error message for this in
clang/include/clang/Basic/DiagnosticSemaKinds.td so I have added a new
one. I'm not sure if it in the right place and the formulation might be
improved.

Fixes #48755

Diff Detail

Event Timeline

massberg created this revision.Dec 6 2022, 12:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 12:55 AM
massberg requested review of this revision.Dec 6 2022, 12:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 12:55 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
massberg updated this revision to Diff 480369.Dec 6 2022, 12:59 AM

Fix test.

shafik added a subscriber: shafik.EditedDec 6 2022, 7:36 AM

It looks like we have an existing message for the regular function case e.g.:

void g() {
    int x;
    void f(int y = x);

    auto lambda1 = [] <auto y = x> {}; 
}

The diagnostic is :

error: default argument references local variable 'x' of enclosing function
    void f(int y = x);
                   ^

see godbolt: https://godbolt.org/z/1vneM5b35

shafik added a comment.Dec 6 2022, 7:45 AM

It looks like the existing diagnostic is issued by CheckDefaultArgumentVisitor. I think you can reuse it here but not 100% on that.

It looks like the existing diagnostic is issued by CheckDefaultArgumentVisitor. I think you can reuse it here but not 100% on that.

Thanks for the pointer! I will have a look.

massberg updated this revision to Diff 481264.Dec 8 2022, 6:30 AM

Use isLocalVarDecl to check if we have a local variable.

massberg updated this revision to Diff 481295.Dec 8 2022, 8:43 AM

Reuse existing error message.

It looks like we have an existing message for the regular function case

Thanks! We can indeed reuse this error message.

It looks like the existing diagnostic is issued by CheckDefaultArgumentVisitor. I think you can reuse it here but not 100% on that.

Thanks for the pointer! CheckDefaultArgumentVisitor is for default arguments of functions. As it does many additional checks we cannot reuse it for testing default arguments of templates.

shafik added inline comments.Dec 8 2022, 12:29 PM
clang/lib/Sema/SemaTemplate.cpp
1593

So if we look at CheckDefaultArgumentVisitor::VistDeclRefExpr it has a comment about having to check ODR use due to cwg 2346 and I believe you should be using that as well.

Which would make your example below pass since the variable is const see: https://godbolt.org/z/78Tojh6q5

It looks like neither gcc nor MSVC implement this DR.

shafik added inline comments.Dec 8 2022, 12:47 PM
clang/test/SemaTemplate/default-template-arguments.cpp
10

To clarify my comment about, I think w/ the ODR use checking this should pass.

shafik added inline comments.Dec 8 2022, 1:03 PM
clang/lib/Sema/SemaTemplate.cpp
11

Please remove.

massberg updated this revision to Diff 482140.Dec 12 2022, 8:24 AM

Additionally check for ODR use and remove inlcude added by accident.

massberg marked 2 inline comments as done.Dec 12 2022, 8:34 AM
massberg added inline comments.
clang/lib/Sema/SemaTemplate.cpp
11

Ups, thanks!

1593

Thanks! I have extended the check.

However, the error is still shown for the const case in the test.
When comparing with your function example, I observed that the difference is that cone an int and otherwise an auto is used.
I haven't expected a difference, but with auto the existing check for local variables as parameters of functions shows an error in case of
auto instead of int: https://godbolt.org/z/Kvfvn4aEr (Is this a bug?).

I have extended the test cases to have cases with auto and cases with int, but even in the int case the code still prints an error.

clang/test/SemaTemplate/default-template-arguments.cpp
10

Thanks! See my comment above.

shafik added inline comments.Dec 12 2022, 2:25 PM
clang/lib/Sema/SemaTemplate.cpp
1593

I am bit surprised that they are considered ODR used in the case you added to the test. I am reading the standard and I don't see it but I need to dig some more to figure out if I am missing something.

tahonermann added inline comments.
clang/test/SemaTemplate/default-template-arguments.cpp
12

MSVC and Clang currently accept x_static. The resolution of CWG 2346 suggests that this case (and maybe x_constexpr) should be accepted following the adoption of P0588R1 (Simplifying implicit lambda capture). However, it isn't obvious to me that P0588R1 actually addresses this. It might be worth following up with CWG to understand what the intent is.

shafik added inline comments.Dec 13 2022, 4:08 PM
clang/test/SemaTemplate/default-template-arguments.cpp
12

I posted to the core reflector on this topic and there is already some feedback but let me wait to see if there is a consensus of this will need further discussions in core.

massberg marked 2 inline comments as done.Fri, Jan 20, 12:06 AM
massberg added inline comments.
clang/test/SemaTemplate/default-template-arguments.cpp
12

Hi! Is there any update on this? How should I proceed?