Dart and analyzer treat name conflict in the different ways

[iarkh]

Dart SDK version: 2.14.4 (stable) (Wed Oct 13 11:11:32 2021 +0200) on "windows_x64"

The following source code example should throw compile errors:

    class C {
      int s1 = 1;
      int s2 = 1;

      static set s1(var value) => throw "Should not reach here";
      static String s2() => throw "Should not reach here";
    }

    main() {}

It throws 2 compile errors with analyzer and 3 compile errors + 1 warning with dart.

So is this a correct result or both tools should produce the same?

There are two kinds of conflicts here, because two instance variables implicitly induce a getter and a setter, so we have the conflict specified in 'Class Member Conflicts' as well as a regular name clash.

Analyzer reports the 'Class Member Conflicts' error, and it seems reasonable to omit the plain name clashes. Sample output is:

    Analyzing test.dart...
      error - Class 'C' can't define static member 's1' and have instance member 'C.s1' with the same name. - test.dart:5:14 - conflicting_static_and_instance
      error - Class 'C' can't define static member 's2' and have instance member 'C.s2' with the same name. - test.dart:6:17 - conflicting_static_and_instance
    2 errors found.

With dart we get the plain name clash, and not the 'Class Member Conflicts' error for s1. For s2, this is again the plain name clash, although via the implicitly induced members. Sample output is:

    test.dart:6:17: Error: 's2' is already declared in this scope.
      static String s2() => throw "Should not reach here";
                    ^^
    test.dart:3:7: Context: Previous declaration of 's2'.
      int s2 = 1;
          ^^
    test.dart:5:14: Error: Conflicts with the implicit setter of the field 's1'.
      static set s1(var value) => throw "Should not reach here";
                 ^
    test.dart:2:7: Error: Conflicts with setter 's1'.
      int s1 = 1;
          ^

[lrhn]

There are no rules saying how we report compilation errors, just that there must be one in the case where we have a conflict. So, from a specification perspective, anything that prevents compilation is correct.

If consistency is a goal (and because it makes testing easier), we may want to be agree on the errors we show.

The front end seems to be more informative than the analyzer because it points out both declarations that are conflicting. It's not internally consistent, though, since it uses Context for one and Error for another.

[eernstg]

The question might be whether we expect the number of errors reported to be specified.

In this case we have a direct name clash because the instance variable declaration named s1 implicitly induces two declarations named s1 and s1=, and the static setter declaration has name s1=. At the same time, we have an error because the interface of C includes member signatures named s1 and s1=, and we have a static member s1=. The former error occurs because the instance variable is declared in C, but the latter error would also have occurred if the instance variable had been inherited. Those two errors are specified in different locations in the specification (here and here).

But it may seem overly formalistic to require that both of these errors are reported, because they're "about the same name clash" for anyone who isn't spending their time writing language tests. ;-)

[lrhn]

The question might be whether we expect the number of errors reported to be specified.

We don't. We don't actually require errors to be reported at all, just that compilation fails. Failing to compile invalid programs is a language feature. Reporting errors is a usability feature. We want to report errors to help users fix them, and that goal should guide what we do.

We have error tests in tests/language because we want to be sure that we consistently report errors, which actually check that we do report each error, but that's for regression testing, not compliance testing. If the tests start failing because we change the error output to something better, we should fix the tests.

[bwilkerson]

The front end seems to be more informative than the analyzer because it points out both declarations that are conflicting. It's not internally consistent, though, since it uses Context for one and Error for another.

The presentation could be improved to make this more clear, but the front end isn't reporting two diagnostics. It's reporting a single diagnostic (labeled as 'Error') and one context message (labeled as 'Context'). A context message is additional information that is associated with, and provides context for the user to understand, a diagnostic.

In my opinion, it would be better if the context message were indented under the diagnostic in order to make the relationship more clear. It would also be better to not print 'Context' at all, and especially not in a way that makes it look like another level of severity.

But I agree, the analyzer really ought to also provide a context message in this case so that it's easier for users to locate the other declaration that's causing a conflict.

But it may seem overly formalistic to require that both of these errors are reported, because they're "about the same name clash" for anyone who isn't spending their time writing language tests. ;-)

In my mind it's less about being "formalistic" and more about providing the best UX possible. Rather than look at it in terms of how many places the code is violating the specification, I think we want to look at it in terms of how many things the user needs to change in order to bring the code into compliance with the specification. If an instance and static member conflict, then there's really only one problem that needs to be fixed (with several possible ways that the user might choose to fix it).

[eernstg]

@lrhn wrote:

We don't actually require errors to be reported at all,

We do have this, which is awfully close to requiring that we report each error (even though you're right that the error message could be completely uninformative as far as the specification is concerned, as in `There is a compile-time error on line 456 column 12.'):

A Dart implementation must provide a static checker that detects and reports exactly those situations this specification identifies as compile-time errors, and only those situations.

@bwilkerson wrote:

providing the best UX possible

Right, that was also my point: It is probably not helpful to report both kinds of errors in this case—that is, 4 errors rather than 2 errors for the given code snippet—because the two 'Class Member Conflicts' errors target the exact same locations as the two plain name clash errors, and it takes a rather formalistic mind to see that there is a difference.

But this raises a slightly broader issue: Do we have a partial order among all compile-time errors such that we can gather a set of errors that apply at a given location, and then just report the most "informative" one?

[bwilkerson]

Do we have a partial order among all compile-time errors such that we can gather a set of errors that apply at a given location, and then just report the most "informative" one?

No. I'm not actually convinced that (a) we could come up with a partial order or (b) that if we did that it would be the case that the diagnostics that ought to be removed would always be reported at the same location as the "informative" one. It would be nice, though, if we could automate some of the process of eliminating follow-on diagnostics.

[eernstg]

eliminating follow-on diagnostics

I'm not quite sure what that means, but I wasn't thinking about error propagation (that is, var x = 1 + true; is an error, and now print(x); could be an error, too). We very likely want to avoid reporting propagated errors, but they would be related by a chain of cause-and-effect, so there's no need for an ordering on the error messages themselves, we just suppress the emission of each cause-and-effect path immediately after one step, and that probably typically done by using a special InvalidType which doesn't cause errors when used, no matter how.

I was thinking about errors that arise independently at the same location (that probably needs to be at overlapping or nested char offset ranges). For instance, if an instance member has both a name clash and an incorrect-override error.

Do we generally want to report all of those, or just the one that has the highest priority in some sense?

And if we generally want to report all of them, why wouldn't we report both the plain name clash and the 'Class Member Conflicts' error in the original example in this issue?

[bwilkerson]

The way I think of it (which might not be right) is that the spec describes two static errors because it needs to do so in order to be complete. But from a user's point of view there's only one problem: the name can't be used for both an instance member and a static member in the same class. The number of diagnostics should equal the number of problems, not the number of places in the spec where a static error is described.

I'm not quite sure what that means ...

I think of a follow-on diagnostic as a diagnostic for a problem that wouldn't exist if some different problem were fixed. But after thinking about this more, I don't think it's relevant because the probably isn't the kind of situation you were thinking about.

I was thinking about errors that arise independently at the same location ... For instance, if an instance member has both a name clash and an incorrect-override error.

Do we generally want to report all of those, or just the one that has the highest priority in some sense?

That's a little harder. If the user solves the name clash by renaming the instance member, then maybe there was only one problem. But if the user solves the name clash by renaming the static member, then it appears that there were two problems. Because we don't know which situation we're in we don't have enough information to make a perfect choice.

I suspect that we generally opt to report both diagnostics at that point, but there's no compelling reason that I can think of to do so.

[eernstg]

We always have the difficulty that name clashes have two (or more) locations, so we may wish to report just one of them and mention the other one as 'context' or something like that. But that means jumping to a conclusion about which one is a mistake (the one that got the error), which would be misleading in about 50% of the cases.

Here is an example where we would probably want to report two errors on the same variable declaration:

abstract class B {
  int s1();
}

class C extends B {
  int s1 = 0; // Errors: Name clash with static method plus incorrect override.
  static void s1() {}
}

In this case the analyzer reports three errors:

• A conflict between the field s1 in C and the method s1 in B (this is the incorrect override).
• A conflict between the static method s1 in C and the instance member s1 in C (this is 'Class Member Conflicts').
• A conflict between the static method s1 in C and the instance member s1 in B (also 'Class Member Conflicts').

The CFE reports a single error 's1' is already declared in this scope when it reaches the static member.

So it's a thorny topic. ;-)

[bwilkerson]

I agree, there's no clear right answer here. (Though I do believe that reporting both of the instance member conflicts is overkill.)