NNBD. Analyzer and CFE reports errors on different lines

[sgrekhov]

I'm not sure if this an issue or not, but, report it just in case. Please close it, if it is not an issue.
In some cases analyzer and CFE report errors on different lines of code. For example

class A {}

class C1 {
  A a;
//  ^
// [cfe] unspecified
  String s;
//       ^
// [cfe] unspecified
  C1() {}
//^^
// [analyzer] COMPILE_TIME_ERROR.NOT_INITIALIZED_NON_NULLABLE_INSTANCE_FIELD

}

main() {
  new C1();
}

As you can see analyzer reports errors on constructor but CFE on the fields itself. Is it ok?

dartanalyzer version 2.8.0-dev.18.0

[eernstg]

I believe it would be a requirement which is a little bit too constraining if we say that all diagnostic messages must be associated with exactly the same character positions in the source code. Luckily, I think the test runner doesn't have a problem with annotations where the positions are different.

@johnniwinther, @scheglov, WDYT?

[bwilkerson]

I think that CFE and analyzer should move toward reporting the same diagnostics in the same locations as much as possible/reasonable, because it provides a better UX, but I don't believe that we want the specification to dictate where diagnostics should be reported. I also think that it's going to take a long time to achieve that goal, and there might always be situations in which they will always be different.

Out of curiosity, where does CFE report the error if some constructors initialize the field and others don't:

class C {
  String s;
  C.a(this.s);
  C.b();
}

[sgrekhov]

@bwilkerson current CFE reports the following error

class C {
  String s; // Error: This constructor should initialize field 's' because its type 'String' doesn't allow null.
            // String s;
            //        ^
  C.a(this.s);
  C.b();
}

[scheglov]

I agree with Brian that ideally diagnostics should be the same, but this is a new requirement, and it would take a long time to do if we decide so.

For this specific case I think analyzer's location is better, because the issue in the code is that a specific constructor does not initialize a field. And so, this constructor (or constructors) should be marked.

[johnniwinther]

I agree with @bwilkerson and @scheglov.

For this particular issue the CFE should update its reporting.

[eernstg]

Here's a proposal for a rule of thumb in this area:

If a tool emits a diagnostic message at a location which is suboptimal for some reasonably objective reason, we'll create an issue on that as such; if another tool seems to make a better choice then that's an obvious thing to mention this kind of issue. If several tools make confusing choices there'd be multiple issues, with one 'area-meta' issue to explain the overall topic and a proposed choice. This approach would automatically make the diagnostic locations converge to some extent.

This means that we basically don't care about smaller discrepancies in diagnostic locations per se, but we rely on concrete problems caused by these discrepancies to give us an automatic convergence effect.

How do you like that?

[johnniwinther]

Sounds good.

[bwilkerson]

Sounds good to me too, although I might suggest that we create only the "meta" issue initially as a place to discuss the right solution, then create the appropriate product-specific issues after we've decided how it should be fixed for each product.

[eernstg]

Sorry about hijacking this issue for the diagnostic location topic. I think we've agreed to focus on concrete issues with the location, not on strict consistency.

This issue is then again only about the CFE putting the initialization error on the field, rather than on the offending constructor. ;-)

[scheglov]

Sounds good to me.