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 doesn't recognize [this] when documenting a class member. #58155

Closed
srawlins opened this issue Apr 25, 2020 · 11 comments
Closed
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-false-positive P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@srawlins
Copy link
Member

Here's a repro:

class C {
  /// Returns [this].
  C get c => this;
}

comment_references complains. If this is the same issue as #57783, feel free to close.

@srawlins srawlins added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Apr 25, 2020
@srawlins
Copy link
Member Author

srawlins commented May 20, 2021

@pq you changed comment_references to always flag [this], along with [null], [true], and [false], for #57642. #57642 only calls out true and false, and I wonder if flagging [this] is too harsh.

@jcollins-g can you say offhand whether dartdoc resolves [this], in a doc comment on a class method or extension method, to the class?

@srawlins
Copy link
Member Author

As an aside, there is a hope to document synthetic types like dynamic, void, Never, and maybe even values like null.

@bwilkerson
Copy link
Member

It would seem odd to me for [this] to link to the class declaration because this doesn't refer to the class, it refers to an instance of the class. Also, it doesn't seem very helpful given that the class declaration is usually very close at hand or easy to navigate to from the declaration of the extension. And I'll note that this doesn't have navigation within code either.

If it did link to the class, then where should super link to?

@srawlins
Copy link
Member Author

srawlins commented May 20, 2021

That's fair. I can write things like "this [DartType]" or "`this`" instead of "[this]", when writing a doc comment on a method for DartType.

@pq
Copy link
Member

pq commented May 20, 2021

Hrmmm.

@pq you changed comment_references to always flag [this]

I can't quite remember but my thinking is I was striving for consistency w/ dartdoc so the warning could prevent you from comments that dartdoc wouldn't be able to generate nice links for. I'd be curious what @jcollins-g thinks.

@srawlins
Copy link
Member Author

I'll close; no need to dig in further.

@jcollins-g jcollins-g reopened this Jun 18, 2021
@jcollins-g
Copy link
Contributor

I think [this] should be allowed as a comment reference in general; I'm actually adding support for it where applicable in the new dartdoc lookup system.

Rationale: Instances are not documented, so [this] referring to an instance of a type rather than the type/class itself is a distinction without a difference. [this] also allows you to refer to this class or any subclass, vs a specific class or type. This is useful in documentation when members are inherited -- you may wish to refer to the class the method shows up in vs a literal class.

@bwilkerson
Copy link
Member

This is useful in documentation when members are inherited -- you may wish to refer to the class the method shows up in vs a literal class.

I don't think we can mimic that behavior in an IDE because members are not copied down into subclasses. The documentation will only ever appear in the class to which [this] would link.

Do we have a request for being able to link to the subclass to which the documentation was copied? If not I wonder whether we really want to introduce another incompatibility.

@jcollins-g
Copy link
Contributor

To clarify, dartdoc supports [this] already, I just recently added it to the new lookup system to preserve the behavior.

@srawlins
Copy link
Member Author

@bwilkerson I propose we re-close this. I don't think there is strong desire or capability to link [this], and I can drop support in dartdoc, in order to facilitate the migration to analyzer-resolution.

@bwilkerson
Copy link
Member

I agree.

copybara-service bot referenced this issue May 3, 2024
…[Type].

Work towards dart-lang/dartdoc#3761

The analyzer has never recognized `[this]` as a valid doc comment
reference (and the `comment_references` lint rule has similarly
reported such reference attempts). dartdoc has its own algorithms
for resolving comment references, which we are dismantling in favor
of a single resolution, provided by the analyzer.

We've also decided against adding support in the analyzer (see
https://github.com/dart-lang/linter/issues/2079), so these
reference attempts should be re-written.

Change-Id: I49478bb39f135d87acff4047767743862c6b8551
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/365305
Commit-Queue: Johnni Winther <[email protected]>
Auto-Submit: Samuel Rawlins <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>
copybara-service bot referenced this issue May 3, 2024
Work towards dart-lang/dartdoc#3761

The analyzer has never recognized `[this]` as a valid doc comment
reference (and the `comment_references` lint rule has similarly
reported such reference attempts). dartdoc has its own algorithms
for resolving comment references, which we are dismantling in favor
of a single resolution, provided by the analyzer.

We've also decided against adding support in the analyzer (see
https://github.com/dart-lang/linter/issues/2079), so these
reference attempts should be re-written.

CoreLibraryReviewExempt: Comments only.
Change-Id: I6cc85115186527820bdd38dcfda8f61e35ae3fe9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/365204
Reviewed-by: Nate Bosch <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Lasse Nielsen <[email protected]>
copybara-service bot referenced this issue May 16, 2024
…is`.

Work towards dart-lang/dartdoc#3761

Sibling CL to https://dart-review.googlesource.com/c/sdk/+/365204

The analyzer has never recognized `[this]` as a valid doc comment
reference (and the `comment_references` lint rule has similarly
reported such reference attempts). dartdoc has its own algorithms
for resolving comment references, which we are dismantling in favor
of a single resolution, provided by the analyzer.

We've also decided against adding support in the analyzer (see
https://github.com/dart-lang/linter/issues/2079), so these
reference attempts should be re-written.

TEST=Nope
Change-Id: Ie584a8338d4b203c4dce737769dbf2bd9094a42c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/366881
Auto-Submit: Samuel Rawlins <[email protected]>
Reviewed-by: Ben Konyi <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
copybara-service bot referenced this issue May 16, 2024
Work towards dart-lang/dartdoc#3761

Sibling CL to https://dart-review.googlesource.com/c/sdk/+/365204

The analyzer has never recognized `[this]` as a valid doc comment
reference (and the `comment_references` lint rule has similarly
reported such reference attempts). dartdoc has its own algorithms
for resolving comment references, which we are dismantling in favor
of a single resolution, provided by the analyzer.

We've also decided against adding support in the analyzer (see
https://github.com/dart-lang/linter/issues/2079), so these
reference attempts should be re-written.

CoreLibraryReviewExempt: Comments only.
Cq-Include-Trybots: luci.dart.try:flutter-analyze-try,analyzer-win-release-try,pkg-win-release-try
Change-Id: I38dd476e40e4508fb6bf88c28415ff261bae5f43
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/366880
Reviewed-by: Srujan Gaddam <[email protected]>
Commit-Queue: Srujan Gaddam <[email protected]>
Auto-Submit: Samuel Rawlins <[email protected]>
copybara-service bot referenced this issue May 16, 2024
Work towards dart-lang/dartdoc#3761

Sibling CL to https://dart-review.googlesource.com/c/sdk/+/365204

The analyzer has never recognized `[this]` as a valid doc comment
reference (and the `comment_references` lint rule has similarly
reported such reference attempts). dartdoc has its own algorithms
for resolving comment references, which we are dismantling in favor
of a single resolution, provided by the analyzer.

We've also decided against adding support in the analyzer (see
https://github.com/dart-lang/linter/issues/2079), so these
reference attempts should be re-written.

Change-Id: I0a30a34c9f58bd6c98c6b83e607a973c9ba51ed7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/366900
Commit-Queue: Samuel Rawlins <[email protected]>
Auto-Submit: Samuel Rawlins <[email protected]>
Reviewed-by: Mayank Patke <[email protected]>
copybara-service bot referenced this issue Jun 5, 2024
Work towards dart-lang/dartdoc#3761

Sibling CL to https://dart-review.googlesource.com/c/sdk/+/365204

The analyzer has never recognized `[this]` as a valid doc comment
reference (and the `comment_references` lint rule has similarly
reported such reference attempts). dartdoc has its own algorithms
for resolving comment references, which we are dismantling in favor
of a single resolution, provided by the analyzer.

We've also decided against adding support in the analyzer (see
https://github.com/dart-lang/linter/issues/2079), so these
reference attempts should be re-written.

Change-Id: I872c215a574dc3d04f0708387408d22fdfd14c14
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/366882
Reviewed-by: Daco Harkes <[email protected]>
Reviewed-by: Jake Macdonald <[email protected]>
srawlins referenced this issue in flutter/flutter Jun 17, 2024
Work towards dart-lang/dartdoc#3761

Sibling CL to https://dart-review.googlesource.com/c/sdk/+/365204

The analyzer has never recognized `[this]` as a valid doc comment
reference (and the `comment_references` lint rule has similarly
reported such reference attempts). dartdoc has its own algorithms
for resolving comment references, which we are dismantling in favor
of a single resolution, provided by the analyzer.

We've also decided against adding support in the analyzer (see
https://github.com/dart-lang/linter/issues/2079), so these
reference attempts should be re-written.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [ ] I listed at least one issue that this PR fixes in the description
above.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
@devoncarew devoncarew added analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 18, 2024
@devoncarew devoncarew transferred this issue from dart-lang/linter Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-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

5 participants