Skip to content

[macros] support for non_constant_identifier_names (variables and fields) #59401

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

Closed
Tracked by #59399
pq opened this issue Feb 16, 2024 · 4 comments
Closed
Tracked by #59399

[macros] support for non_constant_identifier_names (variables and fields) #59401

pq opened this issue Feb 16, 2024 · 4 comments
Assignees
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-false-positive linter-new-language-feature linter-set-core linter-set-recommended P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@pq
Copy link
Member

pq commented Feb 16, 2024

No description provided.

@pq
Copy link
Member Author

pq commented Feb 16, 2024

I'm assuming we should try and avoid linting where the augmentation author can't do anything. For example, I expect that Mm should not be linted in the augmented A.

class A {
  void Mm() { }
}

augment class A {
  augment void Mm() { }
}

But I have some open questions.

My sense is we should NOT lint the Ss param in the augmented m since Ss cannot be renamed.

class A {
  void m({String? Ss}) { }
}

augment class A {
  augment void m({String? Ss}) { }
}

But what about an unnamed parameter that matches the augmented parameter name?

class A {
  void m(String? Ss) { }
}

augment class A {
  augment void m(String? Ss) { }
}

Or, more oddly,

class A {
  void m(String? s) { }
}

augment class A {
  augment void m(String? Ss) { }
}

Are these cases worth handling?

Overrides.

It's worth noting that we don't do anything smart for overrides which are similar and produce lints whether the code author can do anything about them or not.

image

I'm pretty sure that's not the best behavior but I guess it comes up rarely enough in practice.

Thoughts on what we should do here?

/cc @bwilkerson @munificent

@bwilkerson
Copy link
Member

I'm assuming we should try and avoid linting where the augmentation author can't do anything.

I agree. I think that's a concrete application of an even more general principle: we shouldn't lint where the author can't do anything to fix it.

For example, I expect that Mm should not be linted in the augmented A.

I agree. They need to change the declaration being augmented first (and rename should rename any augmentations in the process).

But what about an unnamed parameter that matches the augmented parameter name?

As far as I can tell from the proposal it's fine if the names of positional parameter differ, so I'm guessing that we should lint that.

It's worth noting that we don't do anything smart for overrides ...

I think we should stop linting the names of overriding methods and the names of named parameters in overriding methods.

@pq
Copy link
Member Author

pq commented Feb 16, 2024

With https://dart-review.googlesource.com/c/sdk/+/352978, all he cases except for variables and fields (which the parser doesn't support yet) should be covered.

copybara-service bot referenced this issue Feb 20, 2024
@pq pq changed the title [macros] support for non_constant_identifier_names [macros] 🟡 support for non_constant_identifier_names (variables and fields) Feb 20, 2024
@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 29, 2024
@pq pq changed the title [macros] 🟡 support for non_constant_identifier_names (variables and fields) [macros] support for non_constant_identifier_names (variables and fields) Apr 8, 2024
@pq
Copy link
Member Author

pq commented Apr 9, 2024

Fixed w/ 1c461dc

@pq pq closed this as completed Apr 9, 2024
@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 20, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-false-positive linter-new-language-feature linter-set-core linter-set-recommended P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants