Skip to content

Conversation

@HerrCai0907
Copy link
Member

@HerrCai0907HerrCai0907 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

@dcodeIO
Copy link
Member

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

@HerrCai0907
Copy link
MemberAuthor

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
MemberAuthor

ping @dcodeIO@CountBleck

@HerrCai0907
Copy link
MemberAuthor

@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.

@CountBleckCountBleck removed their request for review January 15, 2024 05:32
@HerrCai0907HerrCai0907 changed the title BREAKING CHANGE: strictly check operator overload ambiguitybreak: strictly check operator overload ambiguityApr 7, 2024
@HerrCai0907HerrCai0907force-pushed the operator-overload-check branch 2 times, most recently from 03511df to 67765faCompareApril 7, 2024 05:04
@HerrCai0907
Copy link
MemberAuthor

friendly ping @dcodeIO

@dcodeIO
Copy link
Member

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
MemberAuthor

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

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
MemberAuthor

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.

@HerrCai0907HerrCai0907 changed the title break: strictly check operator overload ambiguityfix: strictly check operator overload ambiguityJun 30, 2024
@HerrCai0907HerrCai0907force-pushed the operator-overload-check branch from 67765fa to ac2de3aCompareJune 30, 2024 14:26
@HerrCai0907
Copy link
MemberAuthor

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?

@HerrCai0907HerrCai0907 merged commit dfc8a65 into AssemblyScript:mainSep 26, 2024
@HerrCai0907HerrCai0907 deleted the operator-overload-check branch September 26, 2024 06:26
Sign up for freeto 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

@HerrCai0907@dcodeIO@CountBleck