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

fix: strictly check operator overload ambiguity #2762

Merged

Conversation

HerrCai0907
Copy link
Member

@HerrCai0907 HerrCai0907 commented Oct 8, 2023

operator overloads are merely an internal mechanism, primarily for string implementation, operator overload can support to compare user-defined class, but it need to be limited.
a == b and b == a should run the same function and get the same result in semantic level. Allowing ambiguity is bug prone.
fixes: #2761

@HerrCai0907 HerrCai0907 requested a review from CountBleck October 8, 2023 13:19
@HerrCai0907 HerrCai0907 force-pushed the operator-overload-check branch from bf4335c to 64425ab Compare October 8, 2023 13:50
@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 HerrCai0907 force-pushed the operator-overload-check branch 2 times, most recently from 03511df to 67765fa Compare April 7, 2024 05:04
@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.

@HerrCai0907 HerrCai0907 changed the title break: strictly check operator overload ambiguity fix: strictly check operator overload ambiguity Jun 30, 2024
@HerrCai0907 HerrCai0907 force-pushed the operator-overload-check branch from 67765fa to ac2de3a Compare June 30, 2024 14:26
@HerrCai0907
Copy link
Member Author

HerrCai0907 commented Jun 30, 2024

I treat it as a fix instead of a break change since it should not break any user code. @dcodeIO WDYT?

@HerrCai0907 HerrCai0907 merged commit dfc8a65 into AssemblyScript:main Sep 26, 2024
15 checks passed
@HerrCai0907 HerrCai0907 deleted the operator-overload-check branch September 26, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operator overload
3 participants