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

comment_references lint doesn't support dartdoc's idea of "in scope" #1142

Open
Hixie opened this issue Aug 21, 2018 · 5 comments
Open

comment_references lint doesn't support dartdoc's idea of "in scope" #1142

Hixie opened this issue Aug 21, 2018 · 5 comments
Labels
false-positive P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@Hixie
Copy link

Hixie commented Aug 21, 2018

As of dart-lang/dartdoc#1153, dartdoc considers pretty much everything it knows about to be in scope regardless of whether it's in scope at the Dart level. Unfortunately, the comment_references lint doesn't follow the same convention. So, for example, packages/flutter/lib/painting.dart in flutter's repo ends up flagging the three terms in the library docs ([TextPainter], [Decoration], etc) even though those are valid. This makes the lint unusable for our purposes, even though it actually catches a bunch of errors so we would love to use it.

@natebosch
Copy link
Member

At the least it would be really nice if it considered exported libraries to be in scope.

@davidmorgan
Copy link
Contributor

Looking at where dartdoc does resolution:

https://github.com/dart-lang/dartdoc/blob/master/lib/src/markdown_processor.dart

it seems highly unlikely that we can replicate this fully in the linter. In particular: there are comments in a few places about things being expensive, and it seems like it might end up pulling in non-local information (i.e., how dartdoc is configured).

"Resolve using the analyzer" is just one step of many :)

I definitely want to find a way to make this lint work, but it seems likely that that includes accepting a cut-down version of dartdoc resolution. We might for example end up with a 'strict mode' for dartdoc that only uses the analyzer for resolution--then improve analyzer resolution until people are happy with it.

Thoughts? This is all purely speculation on my part :)

@pq
Copy link
Member

pq commented Nov 12, 2019

it seems highly unlikely that we can replicate this fully in the linter.

I probably agree but would love to hear from @jcollins-g. We'd also talked about using dartdoc as a library at some point to be able to leverage shared functionality. Regardless, this may well be too expensive even were we able to share it.

Another alternative would be to push this into analyzer proper, allowing clients to (conditionally) ask for doc resolution (or something) but I haven't thought too hard about the ramifications. (Maybe @bwilkerson and @scheglov can say something about that.)

@a14n
Copy link
Contributor

a14n commented Nov 12, 2019

@bwilkerson
Copy link
Member

For context, the comment_references lint was written to use the definition of comment references that existed in the Dart Language Specification at the time (I don't know whether the definition is still there). This was before dartdoc existed, if I recall the timing correctly. That doesn't make the lint useful, but explains why it is the way it is.

I too would like to see the analyzer's and dartdoc's resolution of doc comment references be the same and have been advocating for it for a long time. Janice and I have talked about it several times over the last couple of years.

As I understand it, dartdoc works by defining the scope (for "in scope") based on the package containing the files for which documentation is being generated. In order to construct that scope it needs to examine all of the files in the package and build a model of the code. The lint rule would need to have the same model in order to work the same way. That would be an expensive operation to perform for each file being linted, so we'd need a way to cache that model and incrementally update it as the code is being edited.

That said, we have been working for some time to reduce the amount of data that the analyzer caches in order to reduce the memory footprint of the analysis server. The model is large, and caching it would be counterproductive in terms of memory use.

Perhaps there are other options that I'm not seeing.

@srawlins srawlins added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) false-positive labels Jun 17, 2021
@srawlins srawlins added the P3 A lower priority bug or feature request label Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
false-positive P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

7 participants