[PATCH] D156244: [clang] Do not crash on use of a variadic overloaded operator

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added inline comments.


================
Comment at: clang/test/SemaCXX/overloaded-operator-decl.cpp:64
+class E {};
+void operator+(E, ...) {} // expected-error{{overloaded 'operator+' cannot be variadic}}
+void d() { E() + E(); }
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > cor3ntin wrote:
> > > aaron.ballman wrote:
> > > > Fznamznon wrote:
> > > > > aaron.ballman wrote:
> > > > > > I think it might make sense to extend the test coverage for the other operators you can overload, just to demonstrate we diagnose them all consistently. WDYT?
> > > > > Okay, while trying to add more test cases I discovered that following
> > > > > ```
> > > > > class E {};
> > > > > bool operator<(const E& lhs, ...);
> > > > > auto operator<=>(const E& lhs, ...);
> > > > > 
> > > > > void d() {
> > > > >   E() < E();
> > > > > }
> > > > > ```
> > > > > crashes even with the patch since there is code searching for best overload candidate that doesn't consider possibility for them making variadic.
> > > > > The code around overloading is actually pretty inconsistent, somewhere invalid candidates are considered, and somewhere not, so I spent some time not knowing what to do.
> > > > > I'm now inclined that we just shouldn't consider invalid candidates like @shafik 
> > > > > suggests. WDYY?
> > > > > 
> > > > Overload resolution doesn't need to produce a candidate that's viable to call; C++ lets you resolve to the "best viable function" only to say "and that one wasn't good enough either." e.g., http://eel.is/c++draft/over#match.general-3
> > > > 
> > > > I've not yet spotted anything in http://eel.is/c++draft/over that says invalid declarations should/should not be added to the initial candidate set. I *think* the intention is that if name lookup can find the name, it goes into the candidate set. Then that set is processed to remove functions that are not viable (http://eel.is/c++draft/over.match.viable). Then we find the best viable function from that set.
> > > > 
> > > > I think we should be keeping the function in the candidate set so long as it matches the rules in http://eel.is/c++draft/over.match.viable even if the function is otherwise not viable. Otherwise, correcting an unrelated issue might change overload resolution to find a completely different function. e.g., in my example above, we'd select `void overloaded(int);` as the best viable function, but when the user corrects the `float` function, we'd change to call that instead. I think it's easier to understand what's going on when picking the `float` overload to begin with and saying "but we can't call that because it's busted".
> > > > 
> > > > CC @cor3ntin @hubert.reinterpretcast @rsmith for some extra opinions, as I'm not certain if I'm interpreting the standard correctly or not.
> > > I'm not sure how much the standardese matter here as the declaration of the operators are ill-formed anyway.
> > > But from a QOI perspective, i think keeping in the set everything the users might reasonably expect to be considered makes sense to me, as it *should* lead to better diagnostics
> > Thanks for the extra perspective! Yeah, I think that's what I've been convincing myself of. Effectively:
> > ```
> > int overloaded(int);
> > float overloaded(float) noexcept(error()); // error: invalid declaration
> > 
> > int main() {
> >   (void)overloaded(1.0f); // #1
> > }
> > ```
> > We're always going to get the invalid declaration diagnostic. The question is whether we want a diagnostic at #1 that says "can't call this overload" or whether we want #1 to have no diagnostic (because it picked the `int` overload). From a purely diagnostic perspective, I think I can see arguments either way. But if we change it slightly:
> > ```
> > template <typename Ty>
> > constexpr int func() { return 0; }
> > 
> > template <>
> > constexpr int func<int>() { return 1; }
> > 
> > template <>
> > constexpr int func<float>() { return 2; }
> > 
> > int overloaded(int);
> > float overloaded(float) noexcept(error()); // error: invalid declaration
> > 
> > static_assert(func(overloaded(1.0f)) == 2); // #1
> > ```
> > we still get the invalid declaration diagnostic, but now we get a failing static assertion because we picked the `int` overload rather than the `float` overload. I think this is clearly worse behavior than if we issued an additional diagnostic at #1 saying that the overload we picked (`float`) was invalid.
> > 
> > https://godbolt.org/z/34aGMY43n
> > 
> > WDYT?
> Agreed. If i write an overload i presumably want the compiler to consider it, even if I botched it. And apparently, the issue is not that we don't do that but rather than we don't always do that. We should strive to be consistent.
> 
> It's not critical because even in the `static_assert` case, the only thing a user can do is fix the declaration, at which point they would get the ambiguity diag or the correct best match correctly picked. But it's nicer to have more diags, when we can
I see, thank you both for the extensive explanation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156244/new/

https://reviews.llvm.org/D156244