Parameters shadow properties in dartdocs (lots of broken links in Flutter docs as a result)

[Hixie]

The Opacity constructor looks like this:

  /// Creates a widget that makes its child partially transparent.                                                                                                                                                                                                  
  ///                                                                                                                                                                                                                                                               
  /// The [opacity] argument must not be null and must be between 0.0 and 1.0                                                                                                                                                                                       
  /// (inclusive).                                                                                                                                                                                                                                                  
  const Opacity({                                                                                                                                                                                                                                                   
    Key key,                                                                                                                                                                                                                                                        
    @required this.opacity,                                                                                                                                                                                                                                         
    Widget child,                                                                                                                                                                                                                                                   
  }) : assert(opacity != null && opacity >= 0.0 && opacity <= 1.0),                                                                                                                                                                                                 
       super(key: key, child: child);

   /// ...
   final double opacity;                                                                                                                                                                                                                                             

Our goal in writing this was that the constructor docs would link to the opacity field's page. Instead, it doesn't link anywhere.

It would be much nicer if arguments were not considered "in scope" for [...] links, since they don't actually have their own page to link to, so it doesn't really do any good to link to them.

Technically in the Flutter docs this manifests as "Broken links, widespread", but I guess it's really a mistake on our part (we didn't understand that arguments would be considered "in scope" and would thus prevent the links from working) so it's also "Enhancements considered important but without significant data indicating they are a big win" and I've marked it P2.

[jcollins-g]

The scoping change to arguments is fairly recent (around the time of canonicalization) in order to reduce warnings, as I recall there were many places which had arguments referenced in square brackets. We could alter the scoping rules to only consider arguments if other possibilities fail, reducing their priority in the lookup order.

[Hixie]

Sounds reasonable. For Flutter we have the convention that square brackets shouldn't be used for arguments at all, though I understand this isn't the case for other projects so it may be impractical to implement that uniformly.

[yjbanov]

Today, when there's is a name conflict between a field and a parameter. There is a way to resolve it:

class Foo {
  int foo;

  /// [foo] points to the parameter.
  /// [Foo.foo] points to the field.
  void bar(int foo) {}
}

If we change scoping rules, we should also update the way such conflicts are resolved.

[Hixie]

I would just remove the [...]-linking to arguments entirely. I think if we want to have a way to link to arguments we should have a separate syntax for them.

I would strongly discourage using [Foo.foo] to refer to a property from a method on that class because it makes it look like a constructor (or maybe a static... or maybe suggests Foo is another class...). Certainly it would be weird because it would look so different from [bar] in the same context, despite being identical. It also reads terribly.

[jcollins-g]

Since there's nothing to link to for an argument, providing a way to resolve the conflict so you don't link to something seems like a lower priority to me? After all, if you don't want a link, you'd just not put one there...

Removing the [...]-linking is doable. However as you pointed out earlier, there's not anything to link arguments to anyway. So reducing the priority would have the same impact on the output -- the only difference will be in how this is warned. Reducing the priority will choose to link to a field, etc, when a conflict is detected yet not warn when an argument link is attempted. Removing it entirely will add warnings.

I think what I'm leaning toward is to make the warning system more granular so we can enable and disable specific warnings and implement this by reducing the priority in the parser.

[yjbanov]

@jcollins-g What would the warning system produce in the example above?

[Hixie]

Yeah I'm totally fine with [foo] linking to the argument if there's nothing else it could plausibly link to. Yegor on a thread internally has been arguing that he wants a way to unambiguously link to arguments even if the arguments clash with a property name, hence my suggestion for a separate syntax. But that's not a high priority for me personally.

[jcollins-g]

Expanded the example slightly for clarity.

For completeness, current behavior:

class Foo {
  int foo;

  /// [foo] points to the parameter.             <<< No warning, doesn't link and is recognized internally as a parameter
  /// [baz] points to the parameter.             <<< No warning, doesn't link and is recognized internally as a parameter
  /// [Foo.foo] points to the field.             <<< No warning, links to field
  void bar(int foo, int baz) {}
}

If support for argument linking is removed entirely:

class Foo {
  int foo;

  /// [foo] points to the parameter.           <<< No warning, links to field
  /// [baz] points to the parameter.           <<< warning: can not link [baz]
  /// [Foo.foo] points to the field.           <<< No warning, links to field
  void bar(int foo, int baz) {}
}

If argument linking is deprioritized:

class Foo {
  int foo;

  /// [foo] points to the parameter.             <<< No warning, links to field
  /// [baz] points to the parameter.             <<< No warning, doesn't link and is recognized internally as a parameter
  /// [Foo.foo] points to the field.             <<< No warning, links to field
  void bar(int foo, int baz) {}
}

[Hixie]

FWIW, either of the latter two sounds good to me.

[yjbanov]

Neither of the options offer conflict resolution, which is a regression from the status quo. However, if #1259 is fixed first, then this would not be an issue.

[jcollins-g]

#1259 will require sdk #27570 first, so that's not likely to happen quickly.

Ideally dartdoc and analyzer would have the same idea about what's going on here, with dartdoc inheriting knowledge about that from the analyzer. If there's not a clearly awesome choice here, it might be better to put off trying to hack this up in dartdoc.

[yjbanov]

#1259 will require sdk #27570 first

If I understand correctly, the two issues are unrelated. dart-lang/sdk#27570 is about allowing putting @required on a field rather than a parameter, and #1259 is about putting dartdocs on parameters.

However, now that I think of #1259, I don't think it would completely eliminate the issue. Sometimes you want to refer to multiple parameters in one sentence. #1259 is only about documenting one parameter at a time. Of course, the chance of name conflict is smaller in this situation, it does remove the reliability of the feature. I want to be able to trust dartdoc links 100%, just like I trust Dart's lexical scoping rules.

Example:

/// Adds [a] and [b], and divides the result by `2.0`.
average(double a, double b) => (a + b) / 2.0;

Even with #1259 fixed, I'd still want to be able to refer from the dartdoc of one parameter to another parameter.

Example:

/// Computes the average of two numbers.
average(
  /// Added to [b].
  double a,
  /// Added to [a].
  double b,
) => (a + b) / 2.0;

[yjbanov]

Ideally dartdoc and analyzer would have the same idea about what's going on here

+1000. I'd love to see dartdocs become part of the language and understood by all static analysis tools (code search, navigation, language-aware code highlighting, refactoring, formatting, etc, etc)

[Hixie]

Please don't let the perfect become the enemy of the good. What matters with dartdoc is that the documentation on the Web site be optimal for our developers. That's a completely separate problem from whatever the analyzer does. We already have completely different resolution rules (for example the analyzer doesn't know what's in scope for dartdocs), this is a minor issue compared to that.

If given the choice between "regressing" what dartdoc internally thinks [foo] links to but having working links all over our docs, or continuing the status quo of the links all being broken but dartdoc's internal data structures remaining unchanged, there is no question in my mind that the former is better.

[jcollins-g]

I generally concur with @Hixie in that the regression is the least bad option on the table right now.

[yjbanov]

Please don't let the perfect become the enemy of the good.

Right back at you. What we did is invent our own Flutter-specific dartdoc syntax and now asking the dartdoc tool to change so it works for us. This will make all of non-Flutter dartdocs, some of which we're re-exporting on docs.flutter.io, incompatible with our internal dartdocs.

If given the choice between "regressing" what dartdoc internally thinks [foo] links to but having working links all over our docs, or continuing the status quo of the links all being broken but dartdoc's internal data structures remaining unchanged, there is no question in my mind that the former is better.

I think your logic is broken. The reason our links are broken is because we're not using the syntax and semantics we're given. Had we used it according to the current semantics, we wouldn't have broken links. Maybe the syntax is not ideal for us readability-wise, so sure, let's file feature requests (e.g. I think this.fieldName reads great is I need to resolve a conflict, it's consistent with how you'd do it Dart code), but also, you know, don't let the perfect become the enemy of the good.

[jcollins-g]

@yjbanov To be fair, we've already taken semantics developers made up in the doc comments and changed dartdoc to make them work (for Flutter, the Dart SDK, and Angular). Indeed, at one point during the canonicalization and warning cleanup hackfest I went through every single warning in all three checking for semantics that were made up and implemented high-value ones that seemed reasonable to me, where they didn't introduce regressions that were worse than the original missing/broken links. It's a little excessively bottom-up, but because I had really little direct user feedback on how users would prefer dartdoc to operate I treated violations of existing semantics as feature requests.(*) So I don't think it's a completely unreasonable thing to do if it generally improves the docs.

If the preferred outcome is this.fieldName to resolve conflicts, that's a change that's quite possible to do -- indeed I thought of doing that as part of work related to the canonicalization overhaul and the warnings improvements. But there were few enough places where people had made up that semantic that the change didn't become a priority.

(*) - IMO, until dartdoc's warnings are locked down far enough to be reliably paid attention to by engineers, some level of bottom-up feature request inference is going to be part of the mix in dartdoc.

[yjbanov]

There's quite a bit of this in the code. The Dart SDK seems to have established this trend as it appears a lot in the standard libraries.

[jcollins-g]

I'm slightly favoring the this model now, based on @yjbanov's compelling argument that this is how resolving such a scoping conflict would work in Dart code. Long term, that's the direction I'd like to see comment resolution work. But either way we choose involves some effort. @yjbanov's approach involves fixing a fair number of docs to include [this.thing], and the approach I originally favored and @Hixie suggested may involve confusing users who expect standard Dart scoping rules while saving that effort.

[yjbanov]

To be clear, the [this.foo] approach does not involve fixing existing docs across the larger Dart ecosystem. It does involve fixing Flutter's docs, but only as a follow-up clean-up step (since the change is non-breaking, there's no rush) and only those where:

• we previously used the backtick syntax to refer to parameters - change them to use [foo]
• we previously used [foo] syntax meaning to refer to a field and there is a name conflict to a parameter - change them to use [this.foo].

It does not require fixing the following:

• initializing formals - instead, dartdoc should link to the field, because initializing formals do not contain useful information, such as type or docs; all such information is at the field declaration site.
• [foo]-style references to fields where there are no naming conflicts - they already point to the field according to today's semantics.

[Hixie]

I think our docs would be substantially harder to read if you had to write [this.foo] some times and [foo] other times. Also, it defeats the entire point here. Consider this file:
https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/widgets/icon_theme.dart
The whole point is that the argument must not be null, but we want it to link to the property that defines the argument's semantics.

[yjbanov]

I can't imagine that this. would be hard to read for a Dart developer. That's already how you disambiguate locals from fields in the language. You already know the semantics, there's nothing else to learn. In addition:

• It's part of the style guide to use foo when not needed, so you have to deal with this.foo some times and foo other times everywhere in the code.
• The this.foo syntax can be for dartdoc author and the dartdoc tool, not necessarily for the reader. That is, it does not have to appear in the generated HTML. Just like [link](http://example.com) results in link in the docs, [this.foo] could be turned into just foo in the docs.

[Hixie]

The dartdocs are frequently read in the raw form, so the syntax matters.

I disagree that switching between this.foo and foo all the time is not confusing, but that's subjective, so I won't try to argue the case further. Even if it wasn't confusing, though, that wouldn't remove the other issue, which is that the whole point is to implicitly refer to the arguments, while linking to the properties.

Let's just have a dedicated syntax for the case where you want to refer to the arguments and not the properties. Maybe that can be ˋ‬fooˋ‬, which would conveniently work with our docs as is, maybe that can be [[foo]], whatever.

[gspencergoog]

Sorry to revive this old issue, but I encountered something that this affects: refactoring in the IDE. If I rename a parameter that uses `parameter`, then I have to manually fix up all locations where that occurs, since the analyzer doesn't treat backticks as symbol references (rightly). If we had a fallback arrangement as described above, as the analyzer seems to already use (if I ctrl-click on a symbol in VSCode, it takes me to the field if there's a field, and to the parameter if there's no field), then this would be less error prone.

I'd be in favor of the deprioritized argument linking proposed above, since it matches what the IDEs already do. In combination with #3149, which I just filed, this should disambiguate references on the API docs pages, since plain parameters would not be linked, and constructor parameters that were also fields would still be linked.

[Hixie]

You can't rely on the refactoring tool to correct the documentation anyway. For example, you have to make sure you go through and correct a to an or vice versa, you have to correct jokes that refer to the name in some way, etc.

[gspencergoog]

Yes, of course you still have to proof the result, but it helps reduce the chance of errors quite a lot.

[Hixie]

I find I get fewer errors using global search, auditing every match for the string being renamed, than from refactoring tools (since with the latter I'm too tempted to avoid doing the global search as well, and only review code near where the changes happened, which may not include all the references in prose). But YMMV.