Better nomenclature for literate diagnostic reporting in analyzer

[stereotype441]

In the code review discussions for https://dart-review.googlesource.com/c/sdk/+/445803, some concerns were raised about the nomenclature used. We decided to go ahead with the CL as is, but keep the discussion about nomenclature open and adjust the names later. I'm opening this issue as a place to have the nomenclature discussion. Once we reach a conclusion I'll make a CL changing the names to whatever we agree on.

For reference, the new API for diagnostic reporting looks like this:

    diagnosticReporter.report(
      diagnosticCode
          .withArguments(...) // omitted if diagnostic takes no arguments
          .withContextMessages(contextMessages) // may be omitted
          .at(astNode),
    );

For comparison, the old API looks like this:

    diagnosticReporter.atNode(
      astNode,
      diagnosticCode,
      arguments: [...], // omitted if diagnostic takes no arguments
      contextMessages: contextMessages, // may be omitted
    );

The names introduced in the CL that we might want to reconsider are:

• report: extension method on DiagnosticReporter that reports a diagnostic using the new API
• withArguments: getter implemented by some diagnostic codes that allows specifying template arguments.
  • A suggested alternative was invoke, since we might want some of the parameters to be things other than template arguments (e.g. context messages for diagnostics that should always have them).
• LocatableDiagnosticImpl: type returned by withArguments.
  • A suggested alternative was DiagnosticCodeCallHasArguments.
• DiagnosticWithoutArguments: mixin for diagnostic codes that don't take arguments.
  • A suggested alternative was DiagnosticCodeCallNoArguments.
• LocatableDiagnostic: interface shared by LocatableDiagnosticImpl and DiagnosticWithoutArguments.
  • A suggested alternative was DiagnosticCodeCall.
• withContextMessages: method on LocatableDiagnostic that attaches context messages.
• atOffset: method on LocatableDiagnostic that attaches an offset and length.
• at: method on LocatableDiagnostic that attaches an offset and length based on an AstNode or Token.
• LocatedDiagnostic: type returned by atOffset and at, and accepted by report.

@scheglov, @bwilkerson, @johnniwinther, @srawlins, what do you think?

[srawlins]

I don't have too strong an opinion. I like the symmetry of DiagnosticCodeCallHasArguments and DiagnosticCodeCallHasArguments, but I don't see much else that sways me one way or another. I'm ok with atOffset and at.

LocatedDiagnostic seems odd, but I think I know the angle we're going for, and I can't think of a better name. It's almost like an "instantiated diagnostic," calling back to this builder pattern. Or a "finalized diagnostic."

In that vein, "LocatableDiagnostic" and company could instead be called "DiagnosticTemplate". "DiagnosticTemplateWithArguments" and "DiagnosticTemplateNoArguments."

These are just suggestions, I don't have a strong opinion that we keep the existing names or change them.

[bwilkerson]

... calling back to this builder pattern.

Yes, I also noted in our meeting earlier in the week that this is effectively a builder pattern, but one with the twist that we ended up with what is effectively a builder-builder.

... could instead be called "DiagnosticTemplate".

I like Template, but I like Builder better. These objects really are builders and using that name would more clearly communicate that to users of the API. I don't, however, like either DiagnosticBuilderBuilderor DiagnosticTemplateTemplate.

I have always believed that if it's this hard to find a good name for 'something' that it's often an indication that the 'something' isn't well designed. I'm not certain that that's the case here, but it's a 'design smell' so it got me to thinking.

One of the goals of the current design was to make it hard to make a mistake. If a diagnostic's message needs arguments, then it should be possible to statically catch the problem when the arguments haven't been provided. Same with the location (file and offset). The current API does a good job with those two factors, but leaves out any safety around context messages.

Ideally we'd have an object that knows that the next piece of data needed is arguments (NeedsArguments), and after arguments are provided you'd get either an object that knows it needs context messages (NeedContextMessages) or an object that knows it needs an offset (NeedsOffset). After providing context messages to a NeedContextMessages you'd always get a NeedsOffset, and after providing an offset you'd get back a HasEverything. (With the possibility that you can skip the NeedsArguments and start directly with either a NeedContextMessages or NeedsOffset. The problem is that we can't express the return type of NeedsArguments.withArguments without using a union type (not that using a union type for a return type would be a particularly good API choice even if we could).

We can design an API that will force the user along a linear path where only the starting point on the path can vary, but when it becomes a graph of possible paths then we don't have any reasonable way that I know of to build a really good API.

We might want to re-consider the goal of the current design. While the safety it offers in terms of ensuring that arguments can't be forgotten when they're required is a good goal, it doesn't ensure that context messages are provided. Perhaps a simple builder pattern with a runtime check to assure that all of the required data is provided is the simpler and better API design.

However, if we decide that partial static safety is better than no safety, then I'd go with something along the lines of what Sam suggested: a prefix of DiagnosticBuilder and a suffix indicating what's still required NeedingArguments, WithoutArguments, or something equivalent. The final class in the chain might be something like CompletedDiagnosticBuilder. As for methods, I like report, withArguments, and withContextMessages. The method atOffset has some tradeoffs.

First, as discussed in the meeting, we've had problems before with needing to create diagnostics for files other than the one the diagnostic reporter is expecting (I think this came up wrt parts, and it might be more common with enhanced parts). This led to needing to be able to access the diagnostic reporter for a different file, and I don't know whether we really want to keep that requirement. Konstantin seemed to think that this is no longer a problem, and he might be right, so feel free to ignore it.

Second, LSP doesn't use an offset and length, it uses a line number and column number pair (for both the beginning and the end of the range). That means that we have to do extra work to get the data we need to send to the client. We're getting around this issue today, and we can keep doing so, so it's also not a blocker.

So perhaps atLocation might be better, or even withLocation, which would be more symmetric. But I'm not opposed to atOffset if we think the current design is better.

The method at is a bit terse, so perhaps atEntity (withLocationFromEntity is a bit long, otherwise I'd also suggest it).

[scheglov]

What if we avoid inventing all these additional classes and names?
And context messages are just another form of arguments, sometimes required fixed set, sometimes arbitrary.
So, just as for other arguments, we can make them named and required. No mistakes allowed!

reporter.atNode(
  node,
  diagnosticCode.call(..arguments, contextMessages: [ // when open-ended set of context messages
    contextMessageA, contextMessageB, contextMessageC,
  ]),
);

or

reporter.atNode(
  node,
  diagnosticCode.call(
    ...arguments,
    requiredContextMessageA: contextMessageA,
    requiredContextMessageB: contextMessageB,
  ),
);

[stereotype441]

@srawlins

I don't have too strong an opinion. I like the symmetry of DiagnosticCodeCallHasArguments and DiagnosticCodeCallHasArguments, but I don't see much else that sways me one way or another.

I assume you meant "I like the symmetry of DiagnosticCodeCallHasArguments and DiagnosticCodeCallNoArguments 😃

I agree--those names, along with DiagnosticCodeCall, do a good job of reflecting the class hierarchy relationship among the three of them. I don't love the word "Call" though; it suggests to me that an instance of the class represents an invocation of something, or perhaps the result of an invocation. In the case of DiagnosticCodeCallHasArguments, that's accurate (it's the result of invoking withArguments). But in the case of DiagnosticCodeNoArguments, it's not (it's just the diagnostic code itself).

I'm ok with atOffset and at.

LocatedDiagnostic seems odd, but I think I know the angle we're going for, and I can't think of a better name. It's almost like an "instantiated diagnostic," calling back to this builder pattern. Or a "finalized diagnostic."

In that vein, "LocatableDiagnostic" and company could instead be called "DiagnosticTemplate". "DiagnosticTemplateWithArguments" and "DiagnosticTemplateNoArguments."

Hmm, interesting suggestion. This also has the nice property of reflecting the class hierarchy relationship, and I like that. I guess they're templates in the sense that they're diagnostics waiting for a location to be filled in.

As for the use of the word "Template", I didn't mention this earlier (because it didn't seem contentious at the time), but I currently already have a bunch of generated class names using "...Template" as a suffix: ParserErrorTemplate, CompileTimeErrorTemplate, StaticWarningTemplate, LinterLintTemplate, etc. These are classes that contain .withArguments() methods, and I like calling them templates because they're diagnostics waiting for arguments to be filled in. Eventually I want to combine them all into a single class. It might be confusing for .withArguments() to be declared in a class that's called "...Template" and return an instance of a class called DiagnosticTemplateWithArguments, when those classes are really two different notions of template (a diagnostic waiting for arguments to be filled in vs. a diagnostic waiting for a location to be filled in).

These are just suggestions, I don't have a strong opinion that we keep the existing names or change them.

@bwilkerson

I like Template, but I like Builder better. These objects really are builders and using that name would more clearly communicate that to users of the API. I don't, however, like either DiagnosticBuilderBuilderor DiagnosticTemplateTemplate.

Yeah, agreed. "BuilderBuilder" and "TemplateTemplate" are fun to say, but I don't think they would make much sense to anyone who wasn't in last week's meeting 😃

I have always believed that if it's this hard to find a good name for 'something' that it's often an indication that the 'something' isn't well designed. I'm not certain that that's the case here, but it's a 'design smell' so it got me to thinking.

One of the goals of the current design was to make it hard to make a mistake. If a diagnostic's message needs arguments, then it should be possible to statically catch the problem when the arguments haven't been provided. Same with the location (file and offset). The current API does a good job with those two factors, but leaves out any safety around context messages.

Ideally we'd have an object that knows that the next piece of data needed is arguments (NeedsArguments), and after arguments are provided you'd get either an object that knows it needs context messages (NeedContextMessages) or an object that knows it needs an offset (NeedsOffset). After providing context messages to a NeedContextMessages you'd always get a NeedsOffset, and after providing an offset you'd get back a HasEverything. (With the possibility that you can skip the NeedsArguments and start directly with either a NeedContextMessages or NeedsOffset. The problem is that we can't express the return type of NeedsArguments.withArguments without using a union type (not that using a union type for a return type would be a particularly good API choice even if we could).

Actually I don't think that's a problem, because the classes that declare .withArguments declare it as a getter whose type is given by a generic parameter, and the type supplied for that generic parameter depends on the diagnostic code. For example, CompileTimeErrorTemplate looks like this:

final class CompileTimeErrorTemplate<T extends Function>
    extends CompileTimeErrorCode {
  final T withArguments;

  const CompileTimeErrorTemplate(...);
}

And the declaration of the individual diagnostic codes looks like this:

  static const CompileTimeErrorTemplate<
    LocatableDiagnostic Function({required String kind, required String name})
  >
  abstractSuperMemberReference = CompileTimeErrorTemplate(...);

So having some .withArguments methods return different types would totally be possible.

We can design an API that will force the user along a linear path where only the starting point on the path can vary, but when it becomes a graph of possible paths then we don't have any reasonable way that I know of to build a really good API.

We might want to re-consider the goal of the current design. While the safety it offers in terms of ensuring that arguments can't be forgotten when they're required is a good goal, it doesn't ensure that context messages are provided. Perhaps a simple builder pattern with a runtime check to assure that all of the required data is provided is the simpler and better API design.

However, if we decide that partial static safety is better than no safety, then I'd go with something along the lines of what Sam suggested: a prefix of DiagnosticBuilder and a suffix indicating what's still required NeedingArguments, WithoutArguments, or something equivalent. The final class in the chain might be something like CompletedDiagnosticBuilder.

Interesting. I like where you're going with this, and I'm trying to see if I can apply it to the current design, where we don't have a class for "needs contexts messages" (but might add one in the future).

A tricky bit is figuring out what to do with LocatableDiagnostic, LocatableDiagnosticImpl, and DiagnosticWithoutArguments. They all represent the "needs offset" state; the differences are:

• LocatableDiagnostic is the abstract base class for by the other two; it declares the atOffset and at methods.
• LocatableDiagnosticImpl comes from applying arguments to a diagnostic code that requires them.
• DiagnosticWithoutArguments is a diagnostic code that doesn't require arguments, and hence is already in the "needs offset" state.

Perhaps we could do something like this:

• LocatedDiagnostic -> CompletedDiagnosticBuilder (this is the type accepted by report)
• LocatableDiagnostic -> DiagnosticBuilderNeedsOffset (defines atOffset and at)
• LocatableDiagnosticImpl -> DiagnosticBuilderNeedsOffsetHasArguments ?
• DiagnosticWithoutArguments -> DiagnosticBuilderNeedsOffsetNoArguments ?

I don't know; those names are getting a bit long for my comfort.

I also confess that I'm not in love with "needs offset", since what it actually needs is an offset and a length, and it might get them from two explicit arguments or from an AstNode or a Token. But I don't have a great suggestion for how to improve on it.

As for methods, I like report, withArguments, and withContextMessages. The method atOffset has some tradeoffs.

First, as discussed in the meeting, we've had problems before with needing to create diagnostics for files other than the one the diagnostic reporter is expecting (I think this came up wrt parts, and it might be more common with enhanced parts). This led to needing to be able to access the diagnostic reporter for a different file, and I don't know whether we really want to keep that requirement. Konstantin seemed to think that this is no longer a problem, and he might be right, so feel free to ignore it.

Second, LSP doesn't use an offset and length, it uses a line number and column number pair (for both the beginning and the end of the range). That means that we have to do extra work to get the data we need to send to the client. We're getting around this issue today, and we can keep doing so, so it's also not a blocker.

So perhaps atLocation might be better, or even withLocation, which would be more symmetric. But I'm not opposed to atOffset if we think the current design is better.

The method at is a bit terse, so perhaps atEntity (withLocationFromEntity is a bit long, otherwise I'd also suggest it).

I don't have strong feelings about this, so let's see what others think.