Errors for name colisions discussion

[FMorschel]

Currently, we have errors like prefix_collides_with_top_level_member and duplicate_definition that are about (respectively) code like:

import 'foo.dart' as foo;
int foo = 0;

int foo = 0;
String foo = '';

These errors are not ideal because they always trigger the first case and not the others.

1. If I am later writing the int foo line, I may want to find where the current foo is defined to decide what to do.
2. If I am later writing the String foo, I may miss the diagnostic because it is far on the screen or (even harder to find) in another part of the library (part files).

This issue is to discuss whether we can do a better job at telling users what we can't resolve in these cases. Maybe even add a "see conflicting" command or something so they can find all cases.

CC @DanTup @bwilkerson

[RohitSaily]

Something that lets one view/jump to other conflicting identifiers would be very convenient as you explained. It may be rare, but we also must consider the cases where there are three or more declarations of the same identifier by showing a list of conflicting definitions like the "see references" feature in VSCode, if possible. This could arise when merging multiple separate code bases at once.

The listing has the benefit of supporting an arbitrary number of conflicts, seeing the context of conflicting declarations without having to jump to them through the preview that comes up, and allowing one to jump to any one if even more context is desired.

[bwilkerson]

These errors are not ideal because they always trigger the first case and not the others.

I am surprised that in the first example the import prefix is being highlighted rather than the declaration. That definitely seems wrong to me, especially the way the specification and problem message are written. I believe that that should be changed. (And the context message as well.

But the second example works fine for me. Even when I add a third top-level declaration (I used void foo() {}) the lexically first declaration has no diagnostic but both of the other declarations have the appropriate diagnostics. The diagnostic has a context message explaining where the first declaration is (and VS Code, at least, lets the user navigate to that location). I don't think there's anything we need to do to duplicate_definition.

[RohitSaily]

As you have said, the second example's error does seem to be working as intended. I believe @FMorschel is suggesting to improve it by also showing the error for the first definition in that case. It currently only shows for the second and beyond. In a large file, if I add a conflicting identifier at the top, I will not see the error on that newly added one in the editor.

int foo = 0; // just added this, it is not shown as an error directly in the text editor (VSCode).

// Many lines here, keeping latter conflicting 
// definitions out of view in the editor
// ...

String foo = ''; // error is fine for this one.

bool foo = true; // error is fine for this one too.

For the first example, ideally we fix the error and also make it so an error is shown on all conflicting declarations in the editor.

[FMorschel]

Exactly as @RohitSaily said. I'm requesting that we allow these two errors (and if we have another similar one I'm not familiar with) to trigger for all definitions of the conflicting top-level members.

This is more problematic when adding new part files - writing part 'other_file.dart'; (it will probably happen more often when we have enhanced-parts). If we have declarations in different files, it is hard to track where all of them are without any help from the diagnostics in question. Letting the user look for all of the declarations by hand seems like unnecessary work when we have the knowledge of all conflicts.

[bwilkerson]

... by also showing the error for the first definition ...

I could see an argument for that based on the fact that the order of the declarations isn't semantically meaningful. That is, given two top-level variables a and b, the order in which the declarations appear doesn't matter, so it's no more correct to say that the second is wrong than it is to say that the first is wrong. However, (a) it seems inconsistent with the way the tools work overall and (b) I think it would be noise more often than it would be signal.

One way to address the inconsistency issue is by applying the same logic in other places. For example, if you import the same library twice in a single library (without prefixes) the tool will flag the second import as being "unnecessary". We could start flagging both as being unnecessary, because there's no valid reason to flag the second rather than the first.

Except, of course, that isn't true. They aren't both unnecessary; at least one of the imports needs to remain. So it really feels like we shouldn't flag both of them. Similarly, it's fine to have one definition of 'foo', it just isn't ok to have multiple of them. And this is what I mean by noise. The tools would be flagging code as wrong when it isn't. Worse than just being noisy, I think that's likely to be confusing to new users of the language.

The current design convention is to accept the lexically first as being valid, to flag all of the remaining cases, and to have a context message pointing to the original declaration. While it's arbitrary, it's as good a choice as any and I think it's the most intuitive for users.

If we have declarations in different files, it is hard to track where all of them are without any help from the diagnostics in question.

I suspect that the case where there are more than two conflicting declarations is very rare. Users will see the conflict as they're adding the second declaration. They'd need to ignore that and then go on to add a third declaration of the same name having already been told that the second declaration wasn't valid. While we should handle this case gracefully, it shouldn't be at the expense of the more common case of having two declarations.

So, for the case where a user just accidentally wrote a second declaration of a name, if the diagnostic appears where they're typing then I think that case works fairly well. If it appears in a file that isn't open, then it doesn't work as well, but it will be visible in the Problems view (assuming that the view is open). If the Problems view isn't open, then this is admittedly a much harder problem.

I think I understand the UX issue now, thank you, but I don't like the proposed solution of displaying a diagnostic on every declaration. I'd like to consider other possible approaches first.

[RohitSaily]

I don't think flagging it as an error would be confusing, it indicates something needs to change in relation to what is flagged, and it isn't necessarily the thing that is flagged. For example

void main()
{	var myNumber=0;
	// ...
	myNumber=2.71828;
}

When I add myNumber=2.71828; the 2.71828 is flagged as an error. It doesn't mean I need to remove/replace it, I could solve the error by changing the type of myNumber to another type such as num or double. In my personal experience, and I would think in many others, this kind of dealing with errors is common.

So I still believe it would be more intuitive to flag the first declaration too, and to change to this approach for all other error checks.

---

If that is not accepted, an alternative idea could be to flag the first declaration as an INFO or WARNING instead of an ERROR and tailoring its message appropriately. I think indicating the declaration's involvement in the error, in some way, is definitely the way to go

[bwilkerson]

In my personal experience, and I would think in many others, this kind of dealing with errors is common.

I agree. The fix isn't always at the site of the diagnostic. And that isn't a goal. The goal is for the highlight range of the diagnostic to draw the user's attention to the location of the problem being reported. (Well, not "the" goal. That's one goal but there are others, such as having a single diagnostic being reported for a single problem in the code.)

So I still believe it would be more intuitive to flag the first declaration too, and to change to this approach for all other error checks.

Are you saying that in

void main()
{	var myNumber=0;
	// ...
	myNumber=2.71828;
}

you would propose generating diagnostics for myNumber at both the declaration site and at every assignment? (I'm guessing we'd continue to use 'invalid assignment' at the assignment site and introduce a 'declaration is not consistent with all use sites' at the declaration site?)

To me that seems completely wrong. The specification doesn't, for example, say that "It's a compile time error to declare a variable v that is assigned a value o somewhere else in the program where o is not assignable to the declared type of v."

I think that in this case there is a valid reason for saying that the assignment is wrong and that the declaration isn't.

... an alternative idea could be to flag the first declaration as an INFO ...

I did consider that possibility, but I don't think that's consistent with the way INFO diagnostics are intended to be used.

I think indicating the declaration's involvement in the error, in some way, is definitely the way to go

While I agree that it's useful to be able to locate the declaration when investigating the invalid assignment, we already have a mechanism for that: context messages.

[RohitSaily]

[...] propose generating diagnostics for myNumber at both the declaration site and at every assignment [...]

What I said was poorly worded because it did not reflect what I was thinking. Flagging every related assignment would definitely be too much. I'm thinking more of flagging the single relevant definitions/declarations for errors that have one. So it would only introduce an additional and direct indicator that something is potentially wrong with the declaration/definition.

[...] mechanism for that: context messages.

From a user experience / visual perspective, it's conventional for that which is flagged (underlined as an error, highlighted as an error, etc) to be something which is the site of an error. When there is some sort of conflict that can directly and easily be remedied by adjusting a definition/declaration, then it makes sense to directly indicate that as a potential error. The context messages are currently useful, in addition to that it could be beneficial to directly show to the user that the definition/declaration needs to be considered as that which is errant.

---

Regarding the specification I will defer its relevancy and interpretation to you and others

[DanTup]

FWIW, TypeScript does flag both locations for a duplicate:

However it doesn't have references between them ("context messages" in Dart).

I do like the idea of flagging both, because if I have only the first on-screen, I can see the problem (and as noted above, it's probably 50/50 about which one is the one I want to keep as-is), however, I don't like polluting the Problems list with "duplicates" because it can already be difficult to work through when some errors cause multiple diagnostics.

I think what would probably be better is if VS Code would indicate the related information itself (for example with a subtle squiggle, and a hover that indicates this range is related to another diagnostic). I've opened an issue about this at microsoft/vscode#248201 - please add 👍 if you think it's a good idea!