Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

break: strictly check operator overload ambiguity #2762

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HerrCai0907
Copy link
Member

fixes: #2761

@dcodeIO
Copy link
Member

dcodeIO commented Oct 8, 2023

Perhaps AS240: Operator '==' overload ambiguity (choosing 'A#__eq' over 'B#__eq').?

@HerrCai0907
Copy link
Member Author

Perhaps AS240: Operator '==' overload ambiguity (choosing 'A#__eq' over 'B#__eq').?

Maybe
AS240: Operator '==' overloading ambiguity (candidate overloads 'operator-overload-ambiguity/A#__eq' and 'operator-overload-ambiguity/B#__eq').

@CountBleck
Copy link
Member

AS240: Ambiguous operator overload == (conflicting overloads 'operator-overload-ambiguity/A#__eq' and 'operator-overload-ambiguity/B#__eq') possibly.

@HerrCai0907
Copy link
Member Author

ping @dcodeIO @CountBleck

@HerrCai0907
Copy link
Member Author

@CountBleck @dcodeIO Could I land it now?
Since it introduces a break change, how can I upgrade minor version for AS? I remember daily publishing action will upgrade patch version.

@CountBleck
Copy link
Member

I can't say all that much about the contents of the PR. @dcodeIO knows what's best.

To upgrade the minor version, you need the BREAKING CHANGE: or major: prefix, so merging the PR as-is should work.

@CountBleck CountBleck removed their request for review January 15, 2024 05:32
@HerrCai0907 HerrCai0907 changed the title BREAKING CHANGE: strictly check operator overload ambiguity break: strictly check operator overload ambiguity Apr 7, 2024
@HerrCai0907
Copy link
Member Author

friendly ping @dcodeIO

@dcodeIO
Copy link
Member

dcodeIO commented Apr 7, 2024

Could this become a warning that mentions which overload has been picked by the compiler? Similar to my prior comment:

AS240: Operator '==' overload ambiguity (choosing 'A#__eq' over 'B#__eq').

Or do you think that there are good reasons to make it an error and hence a breaking change?

@HerrCai0907
Copy link
Member Author

I think we should forbidden this behavior because:

  1. a == b and b == a should be the same thing. It is so crazy if someone gets true from the first expr and gets false from the second.
  2. Since this is an extended grammar, we don't have references from TS. The language which supports operator overloading (I only know cpp) also have similar behavior.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 8, 2024

Given that operator overloads are merely an internal mechanism, primarily for string implementation, and not recommended to be used for anything custom anyway as these won't typecheck, I guess it's fine to check these strictly and error otherwise. More generally, I'd probably even prefer that we limit their use, and encouraged the use of .equals methods or similar in own code, and with it eliminate the ambiguity this PR resolves.

@HerrCai0907
Copy link
Member Author

I'd probably even prefer that we limit their use, and encouraged the use of .equals methods or similar in own code

Agree.
And I guess it will not break any actual code because nobody will consciously write two different comparison function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operator overload
3 participants