Add a warning similar to UNUSED_IMPORT for doc imports that are unused.

[kallentu]

Currently, you can add doc imports to any library whether or not it's used in any references.

/// @docImport 'libb.dart'; // unused and has no warning
library a;

/// Test doc comment
class A {}

It would be nice to have it report UNUSED_IMPORT or UNUSED_DOC_IMPORT when the doc import isn't being used for any reference resolving so we can avoid unnecessary doc imports (and also see that it will actively be used).

cc. @srawlins @goderbauer

[goderbauer]

It should probably also warn if libb.dart is imported as a regular import and hence its doc import is unnecessary/duplicated?

Loosely related: It would also be nice to warn on regular imports if they are only used for a doc reference. The warning should tell people to use a doc import instead.

[bwilkerson]

It would be nice to have it report UNUSED_IMPORT or UNUSED_DOC_IMPORT when the doc import isn't being used ...

Agreed. I'd lean toward making it a new diagnostic (UNUSED_DOC_IMPORT sgtm) because I think that the problem message, correction message, diagnostic documentation, and fixes are all likely to be different.

It should probably also warn if libb.dart is imported as a regular import and hence its doc import is unnecessary/duplicated?

Loosely related: It would also be nice to warn on regular imports if they are only used for a doc reference. The warning should tell people to use a doc import instead.

Yes, and yes.

[FMorschel]

Loosely related: It would also be nice to warn on regular imports if they are only used for a doc reference. The warning should tell people to use a doc import instead.

I think #59741 would be the issue for this.

[FMorschel]

I'll take a look at this to see if I can implement it

[srawlins]

Note that this is, I believe, a tricky one.

Whether imports are used or not depends on the Elements for the imports. And right now doc-imports do not have Elements. They don't participate in the element model.

I tried once to get doc-imports into the element model, in https://dart-review.googlesource.com/c/sdk/+/387861, and it lead to an analysis slow-down for the flutter/flutter repo, so it is reverted. I suspect this is because when doc-imports are included in the calculation of library cycles, the library cycles get huge in flutter/flutter, because they already have hundreds of doc-imports in the codebase. And because the very nature of doc-imports is that you are often doc-importing "downstream" code which depends on the code being documented. So it often creates bigger loops.

It's possible that @scheglov's work on fine-grained dependencies can help here. It might mean that larger library cycles (due to doc-imports) are not as expensive to analyze as they used to be.

[FMorschel]

Alright, thanks for the heads up.

Should we think about other warnings like ambiguous_import too?

[srawlins]

Yeah I think everything like unused import, unnecessary import, duplicate import, unused shown name, unknown hidden name, requires the element model. ambiguous_import on the other hand... I imagine we can implement that today and I neglected to do so 😅

[bwilkerson]

I assume that doc imports have the same problem with resolving a name that might be referring to any of multiple possible elements.

But it isn't clear what the solution would be here. The advice for ambiguous_import is:

Try using 'as prefix' for one of the import directives, or hiding the name from all but one of the imports.

But neither of those is an option for doc imports. So what's a user suppose to do?

[FMorschel]

Possibly stop using doc imports and migrate to a normal import. That is the only way to handle this currently, I think.

[FMorschel]

I assume that doc imports have the same problem with resolving a name that might be referring to any of multiple possible elements.

They just pick the first import they find:

random.dart

class Random {}

main.dart

/// @docImport 'dart:math';
/// @docImport 'random.dart';
///
/// [Random]
library;

If you swap the imports, the hover and go to definition will work as if only the first one exists.

[bwilkerson]

They just pick the first import they find ...

Oh, that's not good. It's inconsistent with the way normal imports work and (as you showed) it means that re-ordering can change what an identifier references, silently leading to bugs in the docs.

[FMorschel]

It is also inconsistent in the sense that if you make the above example with normal imports, the dart -> Random is ignored (there are issues about this behaviour: #58326 and #59566).

import 'dart:math'; // <- unused_import
import 'random.dart';

/// [Random]
void foo() {}

So yes, not good. I'll see what I can do about these warnings, but if you think the team would get this done faster and this should be prioritised, I'd be fine with stepping back.

[bwilkerson]

If you want to look into this you're welcome to do so; I don't think any of us are likely to be able to work on it in the short term.