[clang] Diagnose default arguments defined in different scopes #124844

Endilll · 2025-01-28T21:40:20Z

This patch implements the rule described in N5001.[over.match.best]/4:

If the best viable function resolves to a function for which multiple declarations were found, and if any two of these declarations inhabit different scopes and specify a default argument that made the function viable, the program is ill-formed.

Consequently, it implements CWG1 "What if two using-declarations refer to the same function but the declarations introduce different default-arguments?" and CWG418 "Imperfect wording on error on multiple default arguments on a called function" that were fixed by adding paragraph 4 to [over.match.best].

In the case of friend functions, we no longer diagnose redefinitions of default arguments across scopes. This doesn't change status quo w.r.t. what we accept and what we reject, because such cases also violate [dcl.fct.default]/4.

Background information on default arguments

Sets of default arguments are associated with lexical scope of function declarations they appear in, with a single exception of out-of-line definitions of member function (details below):

For non-template functions, default arguments can be added in later declarations of a function that inhabit the same scope.
Declarations that inhabit different scopes have completely distinct sets of default arguments.

(Note that "inhabit" above is a word of power, which for our case uses the fallback definition in [basic.scope.scope].)
Typically this is the same scope as (semantic) target scope of the declaration, so all redeclarations share the same set of default arguments. However, there are function declarations that (semantically) belong to the different scope than the (lexical) scope they inhabit: extern "C" declarations, local function declarations, friend declarations, and out-of-line definitions of member functions.

Friend declarations and out-of-line definitions of member functions have additional provisions in the wording:

If a friend declaration D specifies a default argument expression, that declaration shall be a definition and there shall be no other declaration of the function or function template which is reachable from D or from which D is reachable.

Except for member functions of templated classes, the default arguments in a member function definition that appears outside of the class definition are added to the set of default arguments provided by the member function declaration in the class definition; the program is ill-formed if a default constructor ([class.default.ctor]), copy or move constructor ([class.copy.ctor]), or copy or move assignment operator ([class.copy.assign]) is so declared.
Default arguments for a member function of a templated class shall be specified on the initial declaration of the member function within the templated class.

Implementation approach

MergeCXXFunctionDecl diagnoses redefinitions of default arguments across function redeclarations in the same scope, in other words, it handles default arguments that are a part of the same set of default arguments. This patch complements it by handling default arguments that are a part of different sets.

[over.match.best]/4 applies when functions are called, so checking it in MergeCXXFunctionDecl would be too early. Sema::BestViableFunction also doesn't fit, because we can skip it if lookup finds a single function. By the time CheckFunctionCall is called, number of arguments in CallExpr is adjusted to include default arguments, so it's too late, too. ConvertArgumentsForCall seems to be the best fit for this check.

cor3ntin

Thanks for working on this, I left a few comments

cor3ntin · 2025-01-29T08:53:07Z

clang/docs/ReleaseNotes.rst

+- Clang now diagnoses ambiguous default arguments declared in different scopes
+  when calling functions, implementing [over.match.best] p4.
+  (`CWG1: What if two using-declarations refer to the same function but the declarations introduce different default-arguments? <https://cplusplus.github.io/CWG/issues/1.html>`_,
+  `CWG418: Imperfect wording on error on multiple default arguments on a called function <https://cplusplus.github.io/CWG/issues/418.html>`_)
+


As an aside should we augment our #GHXXXX logic to support #CWG123 and #LWG123? :D

It would make some sense, but I still like how clickable those links are. #GHXXXX logic optimizes for writing release notes instead of reviewing them, which I find suboptimal.

cor3ntin · 2025-01-29T08:54:27Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+def err_ovl_ambiguous_default_arg
+    : Error<"function call relies on ambiguous default argument %select{|for "
+            "parameter '%1'}0">;
+


Suggested change

def err_ovl_ambiguous_default_arg

: Error<"function call relies on ambiguous default argument %select{|for "

"parameter '%1'}0">;

def err_ovl_ambiguous_default_arg : Error<

"ambiguous default argument %select{|for parameter '%1'}0">;

cor3ntin · 2025-01-29T08:59:03Z

clang/lib/Sema/SemaDeclCXX.cpp

+    if (PrevForDefaultArgs->getLexicalDeclContext()->getPrimaryContext() !=
+            ScopeDC->getPrimaryContext() &&
+        !New->isCXXClassMember())
+      // If previous declaration is lexically in a different scope,
+      // we don't inherit its default arguments, except for out-of-line
+      // declarations of member functions.
+      //
+      // extern "C" and local functions can have default arguments across
+      // different scopes, but diagnosing that early would reject well-formed
+      // code (_N5001_.[over.match.best]/4.) Instead, they are checked
+      // in ConvertArgumentsForCall, after the best viable function has been
+      // selected.
+      continue;
+


Move the comment immediately above the if

we usually quote C++2c rather than a specific draft

but i think the wording we want to quote here is actually
https://eel.is/c++draft/dcl.fct.default#4.sentence-2

There is a "non-template" scenario here too. But we say nothing about member functions in the wording here

do we have tests for that?

struct S { void f(int a = 0); }; void S::f(int a = 2) {}

Move the comment immediately above the if

I'm keeping this consistent with the surrounding code.

we usually quote C++2c rather than a specific draft

Hmm, fine.

but i think the wording we want to quote here is actually
https://eel.is/c++draft/dcl.fct.default#4.sentence-2

No, the wording that you point at (together with the ODR) is what allows MergeCXXFunctionDecl to diagnose redeclarations of default arguments in the same scope early, during declaration matching.

The wording I'm pointing at explains why we need to defer check for default arguments defined in different scopes.

do we have tests for that?

Yes, l3 in default-arguments-different-scopes.cpp.

cor3ntin · 2025-01-29T09:10:26Z

clang/lib/Sema/SemaExpr.cpp

+  // For each such parameter, collect all redeclarations
+  // that have non-inherited default argument.
+  llvm::SmallDenseMap<int, llvm::TinyPtrVector<ParmVarDecl *>> ParamRedecls(
+      LastDefaultArgIndex - FirstDefaultArgIndex + 1);
+  for (FunctionDecl *Redecl : FDecl->redecls()) {
+    for (int i = FirstDefaultArgIndex; i <= LastDefaultArgIndex; ++i) {
+      ParmVarDecl *Param = Redecl->getParamDecl(i);
+      if (Param->hasDefaultArg() && !Param->hasInheritedDefaultArg())
+        ParamRedecls[i].push_back(Param);
+    }
+  }


If you iterate over i in the outer loop, you can forgo the map entirely and do it on one pass

cor3ntin · 2025-01-29T09:10:38Z

clang/lib/Sema/SemaExpr.cpp

+  llvm::SmallDenseMap<int, llvm::TinyPtrVector<ParmVarDecl *>> ParamRedecls(
+      LastDefaultArgIndex - FirstDefaultArgIndex + 1);
+  for (FunctionDecl *Redecl : FDecl->redecls()) {
+    for (int i = FirstDefaultArgIndex; i <= LastDefaultArgIndex; ++i) {


Suggested change

for (int i = FirstDefaultArgIndex; i <= LastDefaultArgIndex; ++i) {

for (int I = FirstDefaultArgIndex; I <= LastDefaultArgIndex; ++I) {

cor3ntin · 2025-01-29T09:27:34Z

clang/lib/Sema/SemaOverload.cpp

+  // [over.match.best]/4 is checked for in Sema::ConvertArgumentsForCall,
+  // because not every function call goes through our overload resolution
+  // machinery, even if the Standard says it supposed to.
+


do we have test for something like that

namespace A { extern "C" void f(long, long = 0); // viable candidate, but not picked } namespace B { extern "C" void f(long, long = 1); } using A::f; using B::f; void f(int) {} // best candidate void g() { f(0); }

ideally we would return OR_Ambiguous here, and then special case the diagnostic for that when all candidates are redeclarations

We have f(0) test in DR tests, but I don't understand how void f(int) {} // best candidate would bring additional coverage. I also don't understand why we'd return OR_Ambiguous.

I think it's worth pointing out that when there is a redeclaration chain, only one declaration out of it is considered a candidate function for overload resolution. At least as far as I saw in the debugger while working on this.

cor3ntin · 2025-01-29T09:28:10Z

clang/test/Parser/function-parameter-limit.cpp

+
+#define PD_10(x) x, x, x, x, x, x, x, x, x, x,
+#define PD_100(x) PD_10(x) PD_10(x) PD_10(x) PD_10(x) PD_10(x) \
+                  PD_10(x) PD_10(x) PD_10(x) PD_10(x) PD_10(x)
+#define PD_1000(x) PD_100(x) PD_100(x) PD_100(x) PD_100(x) PD_100(x) \
+                   PD_100(x) PD_100(x) PD_100(x) PD_100(x) PD_100(x)
+#define PD_10000(x) PD_1000(x) PD_1000(x) PD_1000(x) PD_1000(x) PD_1000(x) \
+                    PD_1000(x) PD_1000(x) PD_1000(x) PD_1000(x) PD_1000(x)
+
+extern "C" int func3( 
+  PD_10000(int = 0)
+  PD_10000(int = 0)
+  PD_10000(int = 0)
+  PD_10000(int = 0)
+  PD_10000(int = 0)
+  PD_10000(int = 0)
+  PD_10000(int = 0) // expected-error {{too many function parameters; subsequent parameters will be ignored}} 
+  int = 0);
+
+int h = func3();


Is that really related to the change?

It was intended as a test for the code that emits diagnostics, but I need to update it, yeah.

Endilll · 2025-01-30T01:04:54Z

During offline discussion with Corentin he pointed out that [over.match.best]/4 talks about multiple declarations that are found, and not simply reachable, like my implementation currently assumes. I'll update it to take the results of name lookup into account.

Endilll added 15 commits January 24, 2025 15:15

[clang] Diagnose default arguments defined in different scopes

da30f70

Add a stress test

a703624

Take extern and local function declarations into account

c911cba

Merge remote-tracking branch 'upstream/main' into cwg418

1a9664e

Fix type in the reference to the Standard

5a76dc9

Leave a comment in BestViableFunction

ce204e4

Handle friend declarations

f47e8cc

Update release notes

bfa5851

Fix stable name in release notes

084fc66

Move the new check function to SemaExpr.cpp where it's now used

e7411f8

Merge remote-tracking branch 'upstream/main' into cwg418

8c27c02

Mark DRs are available in Clang 21

b250d90

Update comment in MergeCXXFunctionDecl

afdf43b

Move the stress test around

344cd4c

I love our coding standard w.r.t. single-line if statements

0e24e37

Endilll added c++ clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 28, 2025

Endilll requested review from cor3ntin, AaronBallman, shafik and Sirraide January 28, 2025 21:40

Endilll self-assigned this Jan 28, 2025

Endilll added 2 commits January 29, 2025 08:50

Merge remote-tracking branch 'upstream/main' into cwg418

2bc0b2a

Add a test with class template

59c9a93

cor3ntin reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Diagnose default arguments defined in different scopes #124844

[clang] Diagnose default arguments defined in different scopes #124844

Endilll commented Jan 28, 2025

cor3ntin left a comment

cor3ntin Jan 29, 2025

Endilll Jan 29, 2025

cor3ntin Jan 29, 2025

cor3ntin Jan 29, 2025

cor3ntin Jan 29, 2025

cor3ntin Jan 29, 2025

cor3ntin Jan 29, 2025

Endilll Jan 29, 2025

cor3ntin Jan 29, 2025

cor3ntin Jan 29, 2025

cor3ntin Jan 29, 2025 •

edited

Loading

cor3ntin Jan 29, 2025

Endilll Jan 29, 2025

cor3ntin Jan 29, 2025

Endilll Jan 29, 2025

Endilll commented Jan 30, 2025

	for (int i = FirstDefaultArgIndex; i <= LastDefaultArgIndex; ++i) {
	for (int I = FirstDefaultArgIndex; I <= LastDefaultArgIndex; ++I) {

[clang] Diagnose default arguments defined in different scopes #124844

Are you sure you want to change the base?

[clang] Diagnose default arguments defined in different scopes #124844

Conversation

Endilll commented Jan 28, 2025

Background information on default arguments

Implementation approach

cor3ntin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cor3ntin Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Endilll commented Jan 30, 2025

cor3ntin Jan 29, 2025 •

edited

Loading