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

#2559. Add augmenting members tests. Part 2 #2799

Merged
merged 3 commits into from
Sep 2, 2024

Conversation

sgrekhov
Copy link
Contributor

@sgrekhov sgrekhov commented Aug 1, 2024

No description provided.

@sgrekhov
Copy link
Contributor Author

@eernstg @chloestefantsova please review

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.

I really don't know where this is going, I hope we can arrive at a clearer model in, for instance, dart-lang/language#4017.

However, right now we'll follow the current spec wording as usual.

Please take a look at the comments. Most of them are background material. Then I'll take another look at the whole thing tomorrow and then we can take the next step.

/// A non-writable variable declaration is augmented with a setter.
///
/// @description Checks that it is a compile-time error if a non-writable
/// variable declaration is augmented with a setter.
Copy link
Member

Choose a reason for hiding this comment

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

Right, so the problem is that we can't augment a setter that doesn't exist, this is an error just like in all the other situations where we try to augment something that doesn't exist.

On the other hand, it should not be a problem to write an introductory declaration of a setter that has the name n= in a situation where we also have a declaration of a getter named n and no setter.

The getter could be the implicitly induced getter of a final variable declaration which is not late-final-without-initializing-expression, or it could be an explicitly written getter, or it could be a variable declaration which is late-final-with-initializer. The point is simply that we'd check that we can add a setter (that one would have to be written explicitly, we don't have any implicitly induced setters that do not also induce a getter).

I suppose we already cover that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we already cover that?
No, some cases were not covered. Added, thank you! Now all of these cases are covered.

/// A non-writable variable declaration is augmented with a setter.
///
/// @description Checks that it is a compile-time error if a non-writable
/// variable declaration is augmented with a setter. Test `late final` variables
Copy link
Member

Choose a reason for hiding this comment

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

Oh, that should not be an error: A late final variable with no initializing expression does have a setter, so we should be able to augment that setter.

But it's true for a late final variable with an initializing expression. One way ahead would be to change the expected response, another could be to add an initializing expression to all these variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Fixed by adding an initializer and also added a separate test that checks that it is not an error to augment late final variable with no initializer with a setter.

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.

Thank you! Updated. PTAL.

/// A non-writable variable declaration is augmented with a setter.
///
/// @description Checks that it is a compile-time error if a non-writable
/// variable declaration is augmented with a setter.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we already cover that?
No, some cases were not covered. Added, thank you! Now all of these cases are covered.

/// A non-writable variable declaration is augmented with a setter.
///
/// @description Checks that it is a compile-time error if a non-writable
/// variable declaration is augmented with a setter. Test `late final` variables
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Fixed by adding an initializer and also added a separate test that checks that it is not an error to augment late final variable with no initializer with a setter.

@sgrekhov sgrekhov requested a review from eernstg August 16, 2024 09:25
@eernstg
Copy link
Member

eernstg commented Aug 23, 2024

This one is on my TODO list, but it is full of situations that give rise to questions that haven't been clarified. I'll look into it on Monday.

@sgrekhov
Copy link
Contributor Author

No problem, thank you!

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.

OK, let's land it!

/// A non-writable variable declaration is augmented with a setter.
///
/// @description Checks that it is a compile-time error if an implicitly induced
/// getter of a final variable declaration is augmented with a setter.
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 there's nothing new about this, it is only targeting the situation where there is no setter to augment (in particular, there is no implicitly induced setter, but also no explicitly written setter).

So there's absolutely nothing in this error message other than the usual "can't augment something that doesn't exist" error.

@eernstg eernstg merged commit e80817a into dart-lang:master Sep 2, 2024
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Sep 10, 2024
2024-09-10 sgrekhov22@gmail.com Fixes dart-lang/co19#2852. Fix the new roll failures (dart-lang/co19#2853)
2024-09-09 sgrekhov22@gmail.com dart-lang/co19#2825. Add scope tests. Part 2. (dart-lang/co19#2847)
2024-09-09 sgrekhov22@gmail.com Fixes dart-lang/co19#2849. Fix new roll failures (dart-lang/co19#2850)
2024-09-09 sgrekhov22@gmail.com dart-lang/co19#2559. Replace augmentation libraries by parts (dart-lang/co19#2848)
2024-09-05 sgrekhov22@gmail.com dart-lang/co19#2825. Add scope tests. Part 1. (dart-lang/co19#2845)
2024-09-04 sgrekhov22@gmail.com dart-lang/co19#2398. Update TouchEvent tests to not fail on platforms where it is not defined (dart-lang/co19#2844)
2024-09-03 sgrekhov22@gmail.com dart-lang/co19#2825. Add import inheritance and grammar tests (dart-lang/co19#2843)
2024-09-02 sgrekhov22@gmail.com dart-lang/co19#2559. Add augmenting members tests. Part 2 (dart-lang/co19#2799)
2024-09-02 sgrekhov22@gmail.com dart-lang/co19#2825. Add enhanced parts tests for top-level declarations and ownership (dart-lang/co19#2842)
2024-09-01 49699333+dependabot[bot]@users.noreply.github.com Bump actions/setup-java from 4.2.1 to 4.2.2 in the github-actions group (dart-lang/co19#2841)
2024-08-30 sgrekhov22@gmail.com dart-lang/co19#2559. Add extension types to augmenting types tests (dart-lang/co19#2839)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try
Change-Id: I26f83579708b877918b5a0a904f8172aa1cf4724
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/384480
Reviewed-by: Erik Ernst <eernst@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
Commit-Queue: Alexander Thomas <athom@google.com>
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