sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Fix priorities are ad-hoc and should be made more regular

Open bwilkerson opened this issue 6 months ago • 1 comments

The analysis server defines constants representing fix kinds. Those constants have a priority associated with them, represented as an integer. There are also constants representing those priorities, but those are published and we don't want to add constants specific to the server's fixes.

As a result, there are now 41 fix kinds whose priority is something like DartFixKindPriority.standard + 6. This makes it difficult to know what the rationale was for using a value other than standard or what value to choose for the next fix.

We should consider defining a server-specific set of priority constants that provides more consistency, and/or defining a set of 'adjustments' that can be consistently applied.

As an example of the latter, we now offer to create declarations of undefined names whether or not the name conforms to the standard naming convention. We have set the priority of convention-matching names higher than the non-convention-matching names and those adjustments could be represented as constants so that the same adjustments are applied uniformly across all declaration kinds.

bwilkerson avatar Jun 09 '25 15:06 bwilkerson

Somewhat related to https://github.com/dart-lang/sdk/issues/60532 (same reasoning made me open that issue).

FMorschel avatar Jun 09 '25 19:06 FMorschel

I think if we change the existing class with an extension type now, as @srawlins suggested at https://github.com/dart-lang/sdk/issues/60532#issuecomment-2824596455:

I think extension types can get you this same result. E.g. type the priority as an extension type, then the user is forced to either wrap each int with the extension type (a bit of extra work, and visually telling), or use an existing one:

extension type Priority(int _it) {
  static Priority high = Priority(10);
}

class C {
  final Priority priority;
  C(this.priority);
}

var x = C(7);

Considering dot-shorthands (I'm not at my PC at the moment so I can't verify easily the constraints on DAS to know if this is active but since there are no big formatting changes, this is probably fine), with use_named_constants this might be a visual help. We could even make this only be a necessity of DAS and not ASP (analysis_server_plugin) by making this extension type implement int and use only a subclass of FixKind that espects it.

FMorschel avatar Nov 13 '25 23:11 FMorschel

I don't understand what you're proposing.

I was just proposing that it would be nice to replace something like DartFixKindPriority.standard + 6 with something like DartFixKindPriority.standard + matchesConventions or DartFixKindPriority.standard + violatesConventions because then the name of the adjustment can reflect the reason for the adjustment.

bwilkerson avatar Nov 14 '25 15:11 bwilkerson

I see that. I'm proposing we do this:

extension type const DartFixKindPriority(int v) implements int {
  static const DartFixKindPriority standard = .new(50);
  static const DartFixKindPriority inFile = .new(40);
  static const DartFixKindPriority ignore = .new(30);

  DartFixKindPriority operator +(int other) => .new(v + other);
  DartFixKindPriority operator -(int other) => .new(v - other);
}

class DartFixKind extends FixKind {
  /// Initialize a newly created kind of fix to have the given [id],
  /// [priority], and [message].
  const DartFixKind(super.id, DartFixKindPriority super.priority, super.message);
}

Or similar. So now, if we have to build a new value like we are doing now, we can migrate things step by step.

First, the above, then we can start making these matchesConventions and violatesConventions constants here and with use_named_constants, we should be set for using the correct values. When that is done, we can change the parameter of the operators above to the extension type, so we always use a named value instead of a magical number.

FMorschel avatar Nov 14 '25 16:11 FMorschel

That feels like a very complex solution to what seems like a simple problem, but YMMV.

bwilkerson avatar Nov 14 '25 16:11 bwilkerson