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

#2641. Add tests for multiple declarations named _ #2656

Merged
merged 5 commits into from
May 14, 2024

Conversation

sgrekhov
Copy link
Contributor

No description provided.

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! I think one location needs to change (it's an unwanted compile-time error), and it would be good to have a bit more on constructors.

LanguageFeatures/Wildcards/binding_A02_t03.dart Outdated Show resolved Hide resolved
@sgrekhov
Copy link
Contributor Author

Fixed (and some other typos) except constructor tear-offs. Please clarify what is expected?

@sgrekhov sgrekhov requested a review from eernstg May 13, 2024 15:17
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! With some tear-off related test cases, it should be fine!

@sgrekhov
Copy link
Contributor Author

Added binding_A01_t11.dart. PTAL

@sgrekhov sgrekhov requested a review from eernstg May 14, 2024 10:13
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! Just one thing remaining: The testing of constructor tear-offs will not reveal the specific faults that I mentioned, as far as I can see. I wrote a rather long comment about this, PTAL.

LanguageFeatures/Wildcards/binding_A01_t11.dart Outdated Show resolved Hide resolved
LanguageFeatures/Wildcards/binding_A02_t03.dart Outdated Show resolved Hide resolved
@sgrekhov
Copy link
Contributor Author

Thank you for the details. I updated the test, please take a look. I'm not sure that we can check that arguments were received correctly in case of wildcards. If I misunderstood you, please correct me

@eernstg
Copy link
Member

eernstg commented May 14, 2024

Ah, you're right: The original constructor has parameters whose name is _, so the corresponding actual arguments must be ignored.

We have a couple of tricky cases where that's not true:

class A {
  final int _;
  A(this._);
}

The language team wants to enable this (I'm at most lukewarm on that), and it should initialize the instance variable whose name is _ (that's not a wildcard). At the same time, this initializing formal does not introduce a local variable into the 'formal parameter initializer scope', which means that A(this._): anotherVariable = _; would be an error.

However, let's postpone tests about this._ and super._ until the language team has concluded something.

(I don't remember, but if we already have some tests about those things then we should just make sure they don't create any noise, e.g., by causing tools to be changed such that they pass a test that may need to be changed.)

Anyway, this is all we can test at this time, so I'll land the PR.

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

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 (fixed a typo).

@eernstg eernstg merged commit 3ba7a38 into dart-lang:master May 14, 2024
2 checks passed
@eernstg
Copy link
Member

eernstg commented May 14, 2024

Issue about super._: dart-lang/language#3792. There is no separate issue about this._ as far as I know.

@sgrekhov
Copy link
Contributor Author

(I don't remember, but if we already have some tests about those things then we should just make sure they don't create any noise, e.g., by causing tools to be changed such that they pass a test that may need to be changed.)

No, we don't have such tests yet. They will be added later as test for Initializing formals part of the spec

@eernstg
Copy link
Member

eernstg commented May 15, 2024

Sounds good, thanks! The language team just decided that super._ and this._ are allowed (super._ when it occurs as a declaration of a positional parameter, not a named one). super._ gives rise to a forwarding semantics in that the actual argument passed to that parameter is also passed to the superconstructor (even though we couldn't write the code to do that because there is no name for that parameter in scope). this._ will initialize an instance variable named _, and it is not possible to reference that parameter by means of _ in the initializer list (but it could be a top-level or static thing, as usual).

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request May 17, 2024
2024-05-16 sgrekhov22@gmail.com dart-lang/co19#2641. Rename `unchanged` -> `other_declarations` (dart-lang/co19#2670)
2024-05-16 sgrekhov22@gmail.com dart-lang/co19#2641. Check that it is still an error to declare more than one class member named `_` (dart-lang/co19#2669)
2024-05-15 sgrekhov22@gmail.com dart-lang/co19#2641. Add tests for multiple top-level declarations named `_` (dart-lang/co19#2667)
2024-05-15 sgrekhov22@gmail.com dart-lang/co19#2641. Add tests for unchanged type declarations named `_` (dart-lang/co19#2666)
2024-05-14 sgrekhov22@gmail.com dart-lang/co19#2641. Add tests for unchanged declarations named `_` (dart-lang/co19#2665)
2024-05-14 sgrekhov22@gmail.com dart-lang/co19#2641. Add tests for multiple declarations named `_` (dart-lang/co19#2656)
2024-05-14 sgrekhov22@gmail.com Fixes dart-lang/co19#2661. Fix numerous typos in augmented_expression_A01_t09 (dart-lang/co19#2664)
2024-05-14 sgrekhov22@gmail.com Fixes dart-lang/co19#2659. Make unique extension members names in augmenting_types_A05_t04 (dart-lang/co19#2662)
2024-05-14 sgrekhov22@gmail.com Fixes dart-lang/co19#2660. Fix typos in augmented_expression_A01_t05 (dart-lang/co19#2663)
2024-05-14 sgrekhov22@gmail.com dart-lang/co19#2641. Add wildcard initializer tests (dart-lang/co19#2658)
2024-05-13 sgrekhov22@gmail.com dart-lang/co19#2119. Remove some accidental comments (dart-lang/co19#2657)

Change-Id: I665f56c1039b14354019a32b94183f6d9c9bc5c7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/367020
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Alexander Thomas <athom@google.com>
Reviewed-by: 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.

None yet

2 participants