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

Introduce a syntax for importing elements just for doc comment references #50702

Closed
srawlins opened this issue Dec 13, 2022 · 37 comments
Closed
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core P3 A lower priority bug or feature request type-design

Comments

@srawlins
Copy link
Member

This is an alternative proposal to #49073. This proposal improves upon the readability and perhaps the simplicity of that proposal. It is based on @lrhn 's brief comment on that proposal.

BACKGROUND

Audience

Anyone writing documented Dart code.

Glossary

  • Doc comment (or dartdoc comment or documentation comment) - a Dart code comment in which each line starts with three slashes (///), and which is placed just before a library element (class, top-level function, top-level constant, etc.) or a class/mixin/enum/extension element (method, field, etc.). A doc comment is parsed (by various tools) as Markdown text.
  • Doc comment reference - the name of any element wrapped in square brackets, inside a doc comment. This is typically one identifier or two separated by a period, like [List] or [Future.wait].
  • Top-level namespace - a set of top-level elements defined by one or more import directives and the current library, which share a common prefix. A library has a set of one or more top-level namespaces.
  • Un-prefixed namespace - the unambiguous union of all elements provided by all of the un-prefixed import directives (no name collisions of non-identical elements are allowed) of a library, plus all elements declared in the library (which may shadow any imported elements).
  • Prefixed namespace - the unambiguous union of all elements provided by all of the import directives which share a common (non-empty) prefix (no name collisions of non-identical elements are allowed).
  • Dart URI resolution - the strategy for resolving a URI string into a path in order to locate a Dart library (or part) file.
  • A Dart library’s export namespace - the collection of Dart elements exported by a library, which includes all public library elements declared in a library (including parts) and all elements exported by the export directives in the library (taking into account any show and hide combinators).

OVERVIEW

The basic design is to add a new annotation, perhaps to the meta package, @docImport which looks similar to an import directive. When a library is annotated with this annotation, the analyzer will add elements to one of a library's top-level namespace, just like an import directive. Doc comment references can then resolve to elements provided by such "doc imports". Quick examples:

@docImport('dart:async')
@docImport('package:flutter/element.dart', show: ['Element'])
@docImport('../path/to/somwhere.dart')
@docImport('dart:html', as: 'html')
library;

/// We can now reference [Future] from dart:async, [Element] from Flutter's element library,
/// and [html.Element] from dart:html, even if none of these libraries are actually imported
/// by this library.
class Foo {}

DETAILED DESIGN/DISCUSSION

A class is introduced which can be used as an annotation:

class docImport {
  final String uri;
  final String? prefix;
  final Set<String> shownNames;
  final Set<String> hiddenNames;
  const docImport(this.uri, {
    String? as,
    Set<String> show,
    Set<String> hide,
  }) :
      this.prefix = as,
      this.shownNames = show
      this.hiddenNames = hide;
}

Each docImport annotating a library directive introduces a doc import which contributes to a new doc namespace.

A library currently has one or more top-level namespaces: the un-prefixed namespace and zero or more prefixed namespaces, which result from import directive(s) with prefixes.

Each identifier in a library (both in code and in doc comment references) is resolved according to various scoping rules, which may resolve to a non-top-level identifier, or a top-level identifier from one of the top-level namespaces. (This is a simplification since a prefix itself is an identifier which needs resolution, and which can also be shadowed by a non-top-level identifier.)

This change introduces a second set of namespaces used only for the resolution of doc comments: the "doc namespaces." The doc namespaces are a superset of the regular namespaces, and each doc namespace is a superset of the corresponding regular namespace (with the same prefix).

For a given library, the doc namespace with prefix p is computed via the following steps:

  1. Start with an empty set of elements.
  2. For each import with prefix p:
    1. For each element provided by the import, according to the rules of 'show' and 'hide':
      • If there is an element with the same name already in the namespace, report a compile-time error.
      • Otherwise, add the element to the namespace.
  3. For each @docImport annotation specified with prefix p:
    1. For each element provided by the import, according to the rules of 'show' and 'hide':
      • If there is an element with the same name already in the namespace, report a static warning.
      • Otherwise, add the element to the namespace.
  4. If this is the un-prefixed doc namespace, then for each top-level element declared in the library:
    1. Add the element to the namespace, replacing any element currently in the namespace with the same name (shadowing it).

(The "regular" namespaces are computed in the exact same way, excluding step 3.)

Each doc comment reference is then resolved using the standard scoping rules, and using the doc namespaces as the set of top-level namespaces.

New static warnings are reported in the following situations:

  • when the URI cannot be resolved, according to the URI-resolving rules
  • when elements from a doc import have name conflicts with each other, or elements from imports with the same prefix
  • when a prefix name does not match certain identifier rules
  • when a show or hide clause includes names of elements which are not found in the library's export namespace.
  • when a doc import is redundant, duplicate, or unnecessary (various subtle definitions) with a regular import or another doc import (lower priority)
  • when a doc import is unused (lower priority)
  • when an element in a show or hide clause of a doc import is unused (lower priority)

ACCESSIBILITY

I am not aware of any accessibility considerations.

INTERNATIONALIZATION

I am not aware of any internationalization considerations.

OPEN QUESTIONS

  1. Can we define the docImport class and constructor in dart:core? This would be required if we want to use docImport functionality in the Flutter engine, which cannot import package:meta.

  2. If not, should we instead support this feature as some sort of directive in a doc comment? For example, instead of the annotations in the OVERVIEW section above, we could instead support:

    /// @doc import 'dart:async';
    /// @doc import 'package:flutter/element.dart' show Element;
    /// @doc import '../path/to/somwhere.dart';
    /// @doc import 'dart:html' as html;
    library;

    This solution has some significant drawbacks:

    • It would be much more complicated to parse all of this syntax in doc comments, making it error-prone.
    • For all of the error cases of the parsing, we'd have to report errors, as long as it really looked like a doc import was intended.
    • We wouldn't get any syntax highlighting for free. That would also have to be implemented, and implemented once for each syntax highlighter we try to keep up (DAS, highlight.js, CodeMirror, Kythe, ...)

TESTING PLAN

Support for this syntax will be tested in:

  • analyzer's unit tests
  • analysis server’s navigation tests, hover tests, etc.
  • dartdoc’s reference unit tests

DOCUMENTATION PLAN

The support for this feature can be documented on the docImport class.

MIGRATION PLAN

If a developer has wanted to reference an out-of-scope element, there are a few strategies they may have ultimately chosen:

  • Use backticks instead. For example, /// See `List` . For this case, we can offer a lint and a quick fix, "this could be a doc comment reference." (This could have many false positives.)
  • Import the element, making it now in-scope, just to be able to reference it. In this case, we could report an "import only used for comment references" lint, with a fix which removes the import and adds a corresponding doc import.
  • Ignore the fact that the IDE won't cross-link, rely on dartdoc's ability to find far-flung elements. For this case, dartdoc will start reporting a warning that such an element will not be discoverable with the current, ambiguous, syntax, and will suggest a doc import. A shell script which parses such warnings could act as a migration tool.

This addresses #44637.

@lrhn
Copy link
Member

lrhn commented Dec 13, 2022

Can we define the docImport class and constructor in dart:core.

We obviously can, but I'd prefer not to.

We generally try to avoid having tool-specific annotations (or annotations at all, since all annotations are tool specific to some extent) in the platform libraries.
Platform libraries are prime real-estate. Adding annotations that makes sense for one tool initially seems reasonable, but when multiple tools starts doing it, it doesn't scale.

Changes to the annotations are driven by the tools, not the library maintainers, which splits the responsibility for maintaining platform libraries into multiple teams.

Because platform libraries are hard to change, it also locks the tools into the current implementation. Changing almost anything becomes a breaking change, which can dampen the velocity of tool development. The tool would be locked to SDK minor-version releases for adding any new features, and major versions (or the breaking-change process) for removing or breaking anything.

A tool can easily support multiple versions of their own metadata at the same time, but the platform libraries
would loathe to have multiple versions, some probably deprecated, at the same time.
Going through a multi-version deprecation, breaking-change and migration process in order to change an annotation isn't really worth it.

We have pragma as the compromise. It's for our own tools, or confident expert uses, to make notes on source code without needing to add visible annotations types to the public namespaces. It's not really for normal end users.
A @pragma("dartdoc:import", "package:foo/foo.dart as p show Foo") can work, but it's not convenient.

(Using a package import to support dartDoc is clearly not viable. It would be adding an import to an otherwise unnecessary library, in order to avoid writing imports to otherwise unnecessary libraries.)

I would recommend using // @doc .... import syntax, and work with the analyzer on recognizing them for highlighting and possibly linking (if that's who controls that, plain regexp-based high-lighting seems doable too).

If enough people think the annotation will be highly valuable to many users, expected to be stable enough to not require frequent updates, and otherwise generally worth it, it's definitely technically possible to add it to dart:core.

We have added other things that are tool-related (like unawaited which only exists because we chose to add the related lint to our recommended set).

@srawlins
Copy link
Member Author

cc @gspencergoog @bwilkerson

If enough people think the annotation will be highly valuable to many users, expected to be stable enough to not require frequent updates, and otherwise generally worth it, it's definitely technically possible to add it to dart:core.

I imagine it being used almost never. It's really just for the flutter/engine and flutter/flutter repos. And some users here and there will use it, but I think an order of magnitude less than unawaited.

That leaves us with:

  • @docImport declared in package:meta and I think it would also have to be declared in dart:ui, so that flutter/engine could use the annotation. The analyzer would have to recognize annotations from both libraries.

  • /// @doc import, which I really prefer not to do for the reasons above, in OPEN QUESTIONS. But it's an option.

@kevmoo
Copy link
Member

kevmoo commented Dec 13, 2022

but I think an order of magnitude less than unawaited.

Less than unawaited maybe, but pretty common. I think I hit these about as often as I hit unawaited.

@gspencergoog
Copy link
Contributor

This doesn't feel like an API that will change much (it mirrors the import mechanism, which is pretty mature syntax), so I don't have a lot of concern about the API needing to wait for major releases to make changes. It's also given meaning by the tools, since it's mostly just metadata, so the tools can evolve around this basic definition without needing the definition itself to change. And, since it's basically the same information as in an import statement, it's not really tool specific: someone could write another documentation system, and they'd still need, and be able to use, this doc import information.

@docImport declared in package:meta and I think it would also have to be declared in dart:ui, so that flutter/engine could use the annotation. The analyzer would have to recognize annotations from both libraries.

I'd prefer it to be in dart:core, for the reasons Sam mentioned initially, but if that's not acceptable, It seems like this is the least bad alternative.

@lrhn
Copy link
Member

lrhn commented Dec 13, 2022

As a counter-point to my other opinion, the one reason to put annotations into platform libraries is that the libraries themselves use those annotations (Deprecated, Since, pragma).

Do we expect that platform libraries will want to use the feature?

Platform libraries may want to refer to, say package:async or package:collection, but definitely do not want to depend on those for compilation. A doc-dependency may be just the thing we need for that.
If we do use an annotation for it, then we will need that annotation in the platform libraries too.
(Obviously, if we use // @doc import ..., then you don't need anything to write it, it's done entirely inside the DartDoc tool. But then, so is the annotation interpretation, it's just the annotation class itself which needs to exist.)

I still think making documentation depend on code-level declarations is breaking out of the documentation. Needing a dependency to write comments (and not just on the thing you want to link to) feels like overkill,

@srawlins
Copy link
Member Author

Do we expect that platform libraries will want to use the feature?

dart:ui is the primary motivation for putting this in the platform libraries. I'll leave it to others to declare whether that counts as being in the platform libraries. As I understand it, the code that makes up dart:ui cannot depend on packages.

@lrhn lrhn added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-design labels Dec 14, 2022
@lrhn
Copy link
Member

lrhn commented Dec 14, 2022

My gut feeling is that the most appropriate approach is to put the links inside comments (maybe even require it to be a doc-comment, so /// @doc import ...). That way, they are only visible to DartDoc itself.

If it's an annotation, then it's visible to everybody, whether they need it or not.
That's true for any other annotation too, but DartDoc already has its own carved out area for non-annotation metadata (doc-comments), so it feels unnecessary to add a second.

I'm not particularly worried about parsing. If we can piggy-back on the Dart parser, it's better, but even without it, it's a fairly restricted grammar.

If we want full IDE integration, then the analyzer should be able to understand the imports too (and therefore parse them), so that it can correctly link external doc-references in an IDE. I'd prefer to have that no matter what syntax we choose.

@srawlins
Copy link
Member Author

To me, dartdoc is only half, maybe less than half of the story here (this proposal hardly references dartdoc). The main client, in my mind, is analyzer, such that these references inside doc comments are resolved correctly inside the IDE (meaning lots of things: navigation, hover, participation in renames, etc), in static analysis, in code search (GitHub, Kythe, etc)

That being said, the singular use is, as you point out, to handle comments, which is not something typical annotation-customers handle. I don't know if the language spec or any other document says what annotations are for, or if their use is encouraged for this purpose or that purpose, but today most annotation use is for code generation (via analyzer APIs) or the analyzer (as its own tool).

@lrhn
Copy link
Member

lrhn commented Dec 15, 2022

The intended use of annotations is to attach metadata to something. The spec doesn't say any more, because it cannot know.
Attaching metadata only makes sense if someone can read it. With dart:mirrors being gone on most real platforms (web and Flutter), annotations are primarily read by source-processing tools.

The design of annotations, however, was guided by being used by mirrors. That's why it's important that the value can be created using actual code imported into the current program, because otherwise the value couldn't exist in the program when dart:mirrors accessed it.
If I were to design metadata today, I'd probably not do it that way. Using Dart objects is a reasonable choice, because it gives structure and type checking that would otherwise become a second mini-language, but I'd probably introduce "meta imports" to back the annotations, which the production compiler would not need to worry about. Members only imported using meta-imports could not be accessed outside of metadata. Maybe even allow more separation, so annotation processing tools only need to care about their own imports, not those of other annotations.
Kind-of like what I think we're doing with macros. And what @docImport would do, but without being reusable for other annotations,.

DartDoc is a little special in this regard in that it's a kind of metadata attached to declarations, intended for a tool to process, that predates annotations. Even if we did have annotations back then, documentation is special, because it's more like text than like structured data. It wouldn't be as convenient if you had to do @dartdoc(""" .... docs ... """) instead.
We'd probably still use comments for documentation, like every other language.

So, annotations are not amazing, because they impose source dependencies that the production code doesn't need.
DartDoc has a way around that by embedding its own metadata inside the comments, like macros, which is why I'd lean into that instead of introducing another external code dependency, just to be able to interpret the doc contents.
In that way doc-imports feel similar to doc macros, which are required to interpret later DartDoc content, but not used for anything else. Doc-macros could also have been annotations, but it didn't seem reasonable (probably also because they contain actual documentation text).

Putting the dependency into dart:core sidesteps adding a package or library dependency, but still moves something related to DartDoc content out of the doc comments where I'd look for it.

@gspencergoog
Copy link
Contributor

gspencergoog commented Dec 15, 2022

I do wish that Dart source files were able to integrate foreign data containers more easily into the language.

For instance, it would be nice to have the ability to segregate portions of the file (without needing to prefix every line) to enable things like building the equivalent of a Jupyter notebook or CoLab, where the code is effectively embedded in the documentation instead of the other way around.

Or being able to have multiple compilation units in one file, so that you can have unit tests, API documentation example code, and application/framework code co-mingled in the same file in different namespaces, and have the analyzer (and thus IDEs) understand all of them and check them in their respective namespaces/containers.

Although, really, that isn't necessarily the purview of the language to define: you could build those things on top of the Dart language as-is with tools (as we've done with dartdoc), and that probably allows better subdivision of responsibility and allows more powerful aggregation of fine grained tools. But it is helpful to be able to attach extra-language metadata in the file to serve as data sources, which is kept in sync with the code, for the tools operate on.

The /// @doc import syntax isn't horrible (although I'd prefer /// @import for brevity), and I can see the argument for "keeping it in dartdoc", but what it means is that the analyzer is going to need to parse and understand not only Dart, but also more and more of the documentation comments as if they were required to be in dartdoc format, making the analyzer more complex than it already is, and importing more of this documentation mini-language into the analyzer. It seems like it would be nice if the documentation formatting/understanding part of the analyzer could be confined to an analyzer plugin so that theoretically multiple different plugins could be created for different documentation sub-languages (don't you want to write API docs in LaTeX or postscript? :-) ).

This extra dependency information seems a little different than the comments themselves, however, and more in the analyzer's wheelhouse to resolve. If you want to aggregate small tools or plugins instead of making the analyzer a monolith, it seems reasonable to provide the different symbol lookup namespaces to a documentation-formatting analyzer plugin as input rather than letting the plugin figure it out for itself from its own metadata.

Maybe a solution that would keep documentation parsing separate from the main analyzer while not involving the @docImport syntax would be to have the analyzer provide an API to plugins for resolving symbols using an alternative set of imports (if that doesn't already exist) after parsing the extra imports out of its own metadata format.

@srawlins
Copy link
Member Author

The /// @doc import syntax isn't horrible (although I'd prefer /// @import for brevity), and I can see the argument for "keeping it in dartdoc", but what it means is that the analyzer is going to need to parse and understand not only Dart, but also more and more of the documentation comments as if they were required to be in dartdoc format ...

Yeah this actually would be all implemented in the parser, just as doc comment references are parsed in the parser. The CFE (I imagine) just drops that stuff on the floor, but it does tokenize it, and we'd apply the same functionality to a doc import directive inside a comment.

Maybe a solution that would keep documentation parsing separate from the main analyzer while not involving the @docImport syntax would be to have the analyzer provide an API to plugins for resolving symbols using an alternative set of imports

I think would be very complicated. We don't provide any APIs for resolving elements.

@lrhn lrhn added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Dec 19, 2022
@pq pq added the P3 A lower priority bug or feature request label Dec 20, 2022
@srawlins
Copy link
Member Author

Thinking and playing in the parser, I have some notes on the doc comment (/// @doc import) support:

  • Currently the Fasta tokenizer tokenizes single-line (/// or //) comments into individual lines: A 3-line doc comment like /// one\n/// two\n/// three\n is tokenized into 3 tokens: /// one\n, /// two\n, /// three\n. There are hooks (if my understanding is correct) for a front-end like analyzer to tokenize more tokens, which is how comment references are broken up into more tokens (analyzer's parseCommentReferences calls scanString to tokenize.

    I think we'll need to tokenize doc imports in the same way. There is a benefit here in that the front_end will (I imagine) not bother tokenizing. Analyzer, on the other hand, can tokenize and parse a doc import.

  • One hiccup here is that a doc import should be able to extend over multiple lines, and the /// bits must be tokenized too:

    /// @doc import 'package:foo/foo.dart'
    ///     as foo;
    /// @doc import 'package:bar/bar.dart'
    ///     show
    ///       One,
    ///       Two,
    ///       Three;

    I imagine we can tokenize the first one, as an example, into @doc import 'package:bar/bar.dart' /// show /// One, /// Two, /// Three; (modulo whether the little bits get their own tokens, like @ or ,, I'm not sure). Notice the /// tokens; not sure how these will be encoded...

    Regardless it means we probably cannot simply tokenize with scanString.

@bwilkerson
Copy link
Member

... modulo whether the little bits get their own tokens, like @ or ,, I'm not sure ...

If we're using the Scanner, which I think we should, then yes, they will be separate tokens, which is appropriate. We'd basically parse until we get to a ;. Of course, we'll need to decide what to do if we don't find a ; (or if there are other parsing errors in the imports).

It makes me wonder just how critical it is for doc imports to be able to extend across multiple lines (or have combinators, prefixes, or configurations, for that matter; I assume we won't support deferred).

Regardless it means we probably cannot simply tokenize with scanString.

We probably can, we'd just need to remove the leading ///s or *s first in order to not scan them as comments.

@srawlins
Copy link
Member Author

@yjbanov: I was thinking about this with @gspencergoog, and our latest idea is writing an annotation class in dart:ui and package:meta. They could both be named @docImport. The one in dart:ui does not need to be exported from any public library, so it can be "internal-only" (even @internal I suppose).

Analyzer can recognize both of the locations of @docImport and treat them identically, to improve doc comment references, and also first class support in the annotation itself. Meaning this code:

@docImport('package:flutter/foundation.dart', show: ['Widget', 'Elemet'])

can have IDE support:

  • hover and jump-to support on the URI
  • hover and jump-to support on 'Widget'
  • error reporting on 'Elemet' (typo)

etc.

@DanTup
Copy link
Collaborator

DanTup commented Feb 2, 2023

our latest idea is writing an annotation class in dart:ui and package:meta. They could both be named @docImport. The one in dart:ui does not need to be exported from any public library, so it can be "internal-only" (even @internal I suppose).

Perhaps a can of worms, but sometimes when I use annotations the server recognises, they remind me of some things in .NET where it didn't matter where an Attribute was defined, only its name. This meant you could often avoid dependencies on extra libraries in your production code by just defining your own internal attribute with the same name.

I've often wondered if the same could work in Dart. For example if I made my own @deprecated and put it on a class, could I avoid needing package:meta? Things may get a little trickier when they have fields, but as long as the analyzer could read them and the rules are clear, maybe it could work?

Each time I see Flutter special cased somewhere, I wonder if someone wanted to make a "Flutter-like" framework, how many places they'd find they can't get the same functionality because of this. Are the reasons that Flutter can't use package:meta particular to Flutter, or might some other project have the same restrictions?

Btw, I'm curious if just using normal imports with some modifier/annotation (@visibleForDocs?) was considered. I suspect it's ruled out for the same reason as not putting the annotation in dart:core, but if this feature is going to have full IDE support for finding references, hovers, renames, etc., it will feel like a first-class Dart feature and having different two ways to "reference" files might feel a little odd.

@srawlins
Copy link
Member Author

srawlins commented Feb 2, 2023

For example if I made my own @deprecated and put it on a class, could I avoid needing package:meta?

Yeah that's nearly what I'm proposing, haha. I think in analyzer we only validate, for example, a @protected annotation if it is the protected const from package:meta, because package:meta is generally available everywhere (hold that thought), and so if some code generator or other defines their own @protected for their own reasons, we should keep our hands off it.

Are the reasons that Flutter can't use package:meta particular to Flutter, or might some other project have the same restrictions?

I'm not sure the technicalities behind it, but basically any dart: library, include "Flutter engine" (dart:ui), cannot reference a package library, like package:meta/meta.dart. @yjbanov actually mentioned that they do import package:meta for their dev cycle, but when the engine is published as dart:ui, all references to package:meta and the various annotations are stripped from the code. That doesn't help us in this case where we want to ship the engine with @docImport annotations intact.

I'm not even sure why the flutter engine is shipped as dart:ui, and not a package library, but I imagine there are good reasons. And if some 3rd party wanted to build their own platform on top of Dart, similar to Flutter, and they shipped their own dart:* library, then yes, they'd have the same problem ☹️ .

Btw, I'm curious if just using normal imports with some modifier/annotation (@visibleForDocs?) was considered. ...

I think the general reason is that IDE support is not good enough to make something a language feature. If it's not even good enough a reason to add to dart:core, I don't see it making it into the language.

@DanTup
Copy link
Collaborator

DanTup commented Feb 2, 2023

and so if some code generator or other defines their own @protected for their own reasons, we should keep our hands off it

Yeah, that's probably fair. I can't remember examples where I'd used this in .NET, but I think they were probably much more specific names than things like @protected so accidentally overlapping was probably much less likely.

Thanks for the extra info!

@devoncarew
Copy link
Member

I'm coming into this conversation a little late, but the idea of using the @pragma annotation appeals to me.

@pragma('dartdoc:import', 'package:flutter/material.dart')

import 'dart:convert';
import 'dart:io';

import 'package:path/path.dart' as p;

...
  • it already exists in our core libraries
  • it's meant to be used for tooling
  • annotations are more parsable than custom conventions in code comments
  • the primary consumer here may not really be dartdoc, but the analyzer / linter / analysis server. Ideally linting and navigation (and code completion?) would be as aware of these auxillary imports, as much as dartdoc itself would be

@lrhn
Copy link
Member

lrhn commented Apr 1, 2023

If possible, would you want to use @pragma('dartdoc:import', '....') as the base, and have a public

class DocImport extends pragma {
  const DocImport(String import) : super('dartdoc:import', import);
}

(If so, I'll have to remove the final from class pragma. So far people haven't been happy about extending pragma.)

@DanTup
Copy link
Collaborator

DanTup commented Apr 1, 2023

If so, I'll have to remove the final from class pragma.

Wouldn't the pattern above be useful even if not used for DartDoc? I didn't know about pragma before, but it seems quite useful and I could imagine that wrapping it like that could be useful for other tools that want to use it.

@lrhn
Copy link
Member

lrhn commented Apr 3, 2023

I'm not sure it's actually useful. I've thought about it some more, and I think it's better to keep pragma as final.

Using a @pragma("dartdoc:import", "...") inside the platform libraries and a @DocImport("...") outside of the platform libraries can be useful, but it's not a given that base class DocImport extends pragma { } is necessary or desirable.

It gives you an implementation approach, which is to detect whether the annotation extends pragma, and then reading its fields for data, making sure to accept both instances of pragma and the subclass DocImport, and maybe even further subclasses. And you'd still check the name property, even on DocImport, because maybe some subclass overwrote the field.

In the same code, you could probably just as easily check for whether the annotation was an instance of one of two final and unrelated classes (pragma or DocImport), only check the name property of the pragma, and you can trust that the fields are not overridden.

So, all in all, I don't think subclassing pragma is the best design choice. It leaves too many things open that you'll then have to check in the annotation recognition code. Going for final everywhere makes that easier.

@DanTup
Copy link
Collaborator

DanTup commented Apr 3, 2023

FWIW, I was expecting checks to be on the name and thought the subclass was nice just for convenience of the user adding the annotation. If I create a tool FooTool and wanted people to use a pragma annotation, wouldn't it be more convenient for them to do:

@fooTool('xyz')

than:

@pragma('FooTool', 'xyz')

Both should work (FooTool should only check name), but the top one gets code completion with a nice dartdoc comment on the completion list explaining how to use it and what it does. The second gets no completion on the important string, and no documentation explaining the use.

(Note: I don't have any current use cases for pragma so is just me thinking out loud - this idea might be flawed in other ways :-) )

@bwilkerson
Copy link
Member

As I mentioned before (#50702 (comment)), scanning across multiple single-line comments isn't a problem. We trim the source of the comments to rescan only the portion between square brackets when dealing with doc references, and this wouldn't be any harder.

@srawlins srawlins self-assigned this Jul 20, 2023
copybara-service bot pushed a commit that referenced this issue Jul 21, 2023
I did not change the content of the functions, except the following:

* modernized some variable declarations with `var`.
* added periods to the end of comments.
* privatized most methods.

Work towards #50702

Change-Id: I4d63d3ee847316b58fa76c12558767c0825027a9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/315243
Reviewed-by: Paul Berry <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
copybara-service bot pushed a commit that referenced this issue Jul 25, 2023
I suspect we ought to just remove many/all of the remaining simpler parser tests. I glanced at a few, and they do not seem specific to analyzer, so their covered features should be covered by parser tests in _fe_analyzer_shared.

#50702

Change-Id: I0e524562f6584145db5e80a154e72ea66090d924
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316042
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
copybara-service bot pushed a commit that referenced this issue Jul 31, 2023
Now that comment reference parsing is done entirely in ast_builder, we
can simplify the implementation.

Work towards #50702

Change-Id: I0650706dfe31542454c7bc9832ec6104c141bd5d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316643
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Paul Berry <[email protected]>
copybara-service bot pushed a commit that referenced this issue Aug 15, 2023
This simplifies comment reference-parsing in this refactor.
Basically when a comment reference is discovered, we
immediately parse it into a CommentReferenceImpl, instead of
passing around the offset from place to place.

Additionally we store fenced code block data on each Comment,
for highlighting purposes. More data will be parsed in this
code in the future. In particular, doc imports.

Work towards #50702

Change-Id: Idb2dcae3fb54af567b1a7f43896c3226644bf6cc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/317446
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
copybara-service bot pushed a commit that referenced this issue Aug 17, 2023
As suggested in https://dart-review.googlesource.com/c/sdk/+/317446.

This introduces a new class CharacterSequence, an abstraction over the
two different doc comment types, and the two remarkably different ways
they need to be walked.

One good example of the complexities is that a line starting with
`// ` must be completely ignored when processing a single-line doc
comment, but must not be ignored when processing the lines of a
multi-line doc comment. Implementing the logic of the two styles in
one body was too ugly and complex.

This also fixes a bug in offsets, as can be seen in the
ast_builder_test.

Work towards #50702

Change-Id: Ife5f17a88268e5cda1ac89d4c14fa1f1cad1333f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/321360
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
copybara-service bot pushed a commit that referenced this issue Aug 30, 2023
Work towards #50702

* This adds a `docImport` field to the `Comment` class.
* Doc imports are parsed with the standard Fasta parser and
  AstBuilder.
* We add one static check that `deferred` isn't used. Other
  checks are needed.
* Many tests.
* We need to add support for line splits in a doc comment.
* I think doc comment imports need to be visted by AST visitors,
  but maybe I am wrong...

Change-Id: I06e2b6fe42ef5ce916d46d9a9db35334726677d0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/322591
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
copybara-service bot pushed a commit that referenced this issue Aug 30, 2023
Work towards #50702

Change-Id: If9c3d702f16e609e0d1f2c2bf09ec02a73cd5a54
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/323427
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
@srawlins
Copy link
Member Author

srawlins commented Jan 4, 2024

Here's an update on what's been implemented, as it's not quite what's specified at top, CC @kallentu:

  • We're going with a comment-based implementation instead of an annotation-based implementation. It looks like:

    /// @docImport 'package:foo/foo.dart';
    /// @docimport 'bar.dart' as bar show Bar;
  • These doc imports are attached to the Comment node, in the docImports field.

  • Doc imports are currently not found on a LibraryElement, but I think they'll need to be.

  • I have a pending CL that incorporates doc imports into the element model, FileState, and more.

Otherwise, I think the resolution steps specified up top, and the new errors that should be reported, are unchanged.

@bwilkerson
Copy link
Member

Doc imports are currently not found on a LibraryElement, but I think they'll need to be.

To clarify, are you talking about having the doc namespace available from the LibraryElement, or do you mean the actual imports (or both)?

I haven't compared closely, but are the rules for doc imports the same as for normal imports (modulo the namespace being computed), or are there some interesting differences?

when elements from a doc import have name conflicts with each other, or elements from imports with the same prefix

I believe that the rules for normal imports is that it's only an error if the conflicting name is actually referenced. I assume the same is true here.

@srawlins
Copy link
Member Author

srawlins commented Jan 5, 2024

I haven't compared closely, but are the rules for doc imports the same as for normal imports (modulo the namespace being computed), or are there some interesting differences?

Yes I think they're exactly the same.

I believe that the rules for normal imports is that it's only an error if the conflicting name is actually referenced. I assume the same is true here.

Ah yes, fair. True here as well.

@srawlins
Copy link
Member Author

@lrhn To complete this work, I think it would be consistent, simple and straightforward for the analyzer will start reporting "compile time errors" on doc-imports, the same that are reported for regular imports. WDYT of that?

For example, /// @docImport 'does-not-exist.dart'; will result in a compile-time error. Is this over-stepping our purview, or good for consistency? We could probably use "warnings" (previously, hints) that mirror such compile-time errors, if we should not actually be reporting compile-time error codes. Something like a dozen different errors can be raised on imports I think.

@lrhn
Copy link
Member

lrhn commented Jan 12, 2024

I'd be fine with reporting an invalid import as an error. I'd also make it an error to have a [Foo] where Foo cannot be resolved.
But I'm usually fine with breaking bad code. Others might be more cautious.
Might want an introductory period, maybe a single release, where it's only a warning with a message saying that in the next release it will become an error.

(I'm a little iffy about calling it a "compile-time error" for a tool that doesn't actually compile, but that's probably just idle pedantry. It's really a "doc-time error", where the analyzer is reporting that that the DartDoc tool will have an error here, just like a compile-time error is the analyzer reporting that a compiler will have an error. Or it's just "an error" because someone wanted to make something not be accepted, aka. lints).

copybara-service bot pushed a commit that referenced this issue Feb 1, 2024
…ors.

This CL adds doc import information to UnlinkedLibraryDirective and reports any static errors such as `URI_DOES_NOT_EXIST` from the doc imports.

The doc imports should be treated similarly to actual imports so it uses a lot of the same logic.

Based largely on Sam's WIP work in this CL:
https://dart-review.googlesource.com/c/sdk/+/344340

Bug: #50702
Change-Id: Ic001fd6d4077eea04905288be0424e7b11b2b56c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/345361
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Kallen Tu <[email protected]>
@srawlins srawlins changed the title Introduce an annotation for importing elements just for doc comment references Introduce a syntax for importing elements just for doc comment references Jun 26, 2024
copybara-service bot pushed a commit that referenced this issue Jun 27, 2024
Work towards supporting #50702
in google3. Related to the work attempted in
flutter/flutter#150798.

Change-Id: I36fb1c47c94d5b965529db910fbd0e347bfba645
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/373442
Reviewed-by: Konstantin Shcheglov <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Sam Rawlins <[email protected]>
@srawlins
Copy link
Member Author

srawlins commented Jul 9, 2024

Superceded by #56186, which includes the new syntax, a directive in doc comments, as implemented in the analyzer.

@srawlins srawlins closed this as completed Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core P3 A lower priority bug or feature request type-design
Projects
None yet
Development

No branches or pull requests

8 participants