Skip to content

Extensions don't trigger library import for their names only #59723

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
FMorschel opened this issue Dec 16, 2024 · 7 comments
Closed

Extensions don't trigger library import for their names only #59723

FMorschel opened this issue Dec 16, 2024 · 7 comments
Labels
devexp-quick-fix Issues with analysis server (quick) fixes legacy-area-analyzer Use area-devexp instead. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@FMorschel
Copy link
Contributor

FMorschel commented Dec 16, 2024

Repro:

extension.dart

extension Ext on int {}

main.dart:

/// This is a dart doc for [f] that uses an [Ext] type.
void f<E extends Ext>() {}

Neither Ext will trigger the quick-fix for the importing of extension.dart. If you add a line Ext; inside f it does.

I volunteer to help with this.

@FMorschel FMorschel added the legacy-area-analyzer Use area-devexp instead. label Dec 16, 2024
@FMorschel
Copy link
Contributor Author

@FMorschel
Copy link
Contributor Author

This still makes one test fail. I'm on my lunch break now, when I get to my computer I'll add a comment on the CL asking for what I should do in that case.

@bwilkerson
Copy link
Member

Note that the use of Ext in void f<E extends Ext>() {} is invalid because Ext isn't a type.

I can see wanting this behavior for the doc comment case, though, but for that case it should probably create a doc-import.

@srawlins

@bwilkerson bwilkerson added P3 A lower priority bug or feature request devexp-quick-fix Issues with analysis server (quick) fixes type-enhancement A request for a change that isn't a bug labels Dec 16, 2024
@FMorschel
Copy link
Contributor Author

FMorschel commented Dec 16, 2024

Yeah, I noticed later. I originally detected this by making a doc. Thanks!

@FMorschel
Copy link
Contributor Author

Currently the LinterLintCode.comment_references calls on ImportLibrary, which adds a normal import.

Adding a call for the forExtension constructor along with forType solves this but doesn't address your point of it being a doc-import. I can probably make this change with more time, but I'd make this CL only about fixing this and creating a new issue/CL for migrating those fixes for this.

Unfortunately, this is only accessible if that lint is enabled and it is off by default. Why is that? I'd expect more care for the docs from everyone and this being on would help with that.

@FMorschel
Copy link
Contributor Author

Updated this CL to the above.

@bwilkerson
Copy link
Member

Unfortunately, this is only accessible if that lint is enabled and it is off by default. Why is that?

One reason is because the lint was written before there were doc imports. If we required users to add a normal import for every reference in a doc comment then the dependency graph would have far more edges than necessary in order to run the program, and that isn't good for users.

As I said elsewhere, we need to look at the lints/warnings for doc imports from a holistic perspective and figure out what the right UX is. It's possible that we'll convert the lint into a warning now that we have doc imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-quick-fix Issues with analysis server (quick) fixes legacy-area-analyzer Use area-devexp instead. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

2 participants