Skip to content

#3054. Add more test cases to closurization tests. Part 2. #3069

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

Merged
merged 4 commits into from
Feb 7, 2025

Conversation

sgrekhov
Copy link
Contributor

@sgrekhov sgrekhov commented Feb 6, 2025

No description provided.

@sgrekhov sgrekhov marked this pull request as draft February 6, 2025 11:36
@sgrekhov sgrekhov marked this pull request as ready for review February 6, 2025 11:44
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

We only have a couple of cases where I'm worried: We shouldn't use tests to prevent optimizations that are reasonable, and that we could otherwise have (now or later).

A a = A();
final fa1 = a.m;
final fa2 = a.m;
Expect.notEquals(fa1, fa2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the spec wording says that the two tear-offs are equal if and only if they are identical. But it doesn't say that they can't be identical.

In this particular case we actually need to preserve the permission to optimize the extension methods: If an extension method doesn't actually depend on the receiver (as in this example: it just returns an argument and never uses this) then it must be OK for an implementation to compile this extension method to, for example, a simple top-level function like num compiled_m(int r1, {String p1 = ""}) => r1; and then return that (simple and constant) function object when the corresponding extension method is torn off.

In this case we'll have identical(fa1, fa2) (and hence also fa1 == fa2).

So we have to weaken the test to say that it's OK for fa1 and fa2 to be unequal, and it's also OK for them to be equal (and even identical, if anyone asks ;-).

As usual, evaluation of fa1 == fa2 shouldn't throw, so we shouldn't just skip the test entirely.

So perhaps this would do?:

Suggested change
Expect.notEquals(fa1, fa2);
Expect.isTrue(fa1 == fa2 || fa1 != fa2);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Makes sense. I simply deleted this test. Expect.isTrue(fa1 == fa2 || fa1 != fa2); will hardly ever fail and will always be true. Therefore, this test does nothing except confirm that extension and extension type methods can be torn off. We have lots of other tests (including ones in this PR) that check this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test that verifies that the operator == on these function objects isn't buggy and throws?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify, how == may throw on such function objects?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a bug, but bugs is the main reason why we have tests.

ET et = ET(0);
final fet1 = et.m;
final fet2 = et.m;
Expect.notEquals(fet1, fet2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar.

A a = A();
final fa1 = a.m;
final fa2 = a.m;
Expect.notEquals(fa1, fa2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, should allow this, but it should also allow optimization that causes fa1 == fa2.

ET et = ET(0);
final fet1 = et.m;
final fet2 = et.m;
Expect.notEquals(fet1, fet2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. PTAL.

A a = A();
final fa1 = a.m;
final fa2 = a.m;
Expect.notEquals(fa1, fa2);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Makes sense. I simply deleted this test. Expect.isTrue(fa1 == fa2 || fa1 != fa2); will hardly ever fail and will always be true. Therefore, this test does nothing except confirm that extension and extension type methods can be torn off. We have lots of other tests (including ones in this PR) that check this.

@sgrekhov sgrekhov requested a review from eernstg February 7, 2025 08:30
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one little thing to think about...

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot!

@eernstg eernstg merged commit 5198824 into dart-lang:master Feb 7, 2025
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 14, 2025
2025-02-13 [email protected] dart-lang/co19#3057. Operator `==` tests renamed (dart-lang/co19#3075)
2025-02-13 [email protected] Fixes dart-lang/co19#3073. Add `asyncEnd()` after `cleanup` invocation in `asyncTest` (dart-lang/co19#3074)
2025-02-07 [email protected] dart-lang/co19#3054. Add more test cases to closurization tests. Part 2. (dart-lang/co19#3069)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try
Change-Id: Ic56a06a8f8ec1fc36b04b5250ad5a4cf698e5404
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/409940
Reviewed-by: Alexander Thomas <[email protected]>
Reviewed-by: Erik Ernst <[email protected]>
Commit-Queue: Alexander Thomas <[email protected]>
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.

2 participants