Redeclaring function parameters gives error on usage not on redeclaration

[DrRuhe]

Describe the issue
When a function parameter is redeclared, an error is displayed on every instance where the variable is used before the redeclaration instead of an error message on the redeclaration.

Relevant linting rules: referenced_before_declaration
To Reproduce

void foo(int tmp) {
  tmp++; // Local variable 'tmp' can't be referenced before it is declared.
  tmp++; // Local variable 'tmp' can't be referenced before it is declared.

  var tmp = 5;
  tmp++;
}

Expected behavior
I would expect the linter to also highlight the redeclaration stating that the variable is already declared.

Additional context
The referenced_before_declaration already gives a hint that this might be the problem ("Try [...] renaming the local variable so that it doesn't hide a name from an enclosing scope."), so why not also highlight the redeclaration as an error, which I would say it is in every case. That means I would also expect the following to throw errors, which at the moment it does not.

void foo(int tmp) {
  var tmp = 12; // no error here
  tmp++;
  print(tmp);
}

[bwilkerson]

The reason we don't simply flag the variable declaration is because it's actually valid in Dart. Dart is a block structured language, which means that variables are defined in scopes, and scopes can be nested in other scopes. As with all block structured languages, it's valid to declare a variable in a nested scope that's also declared in an outer scope; the variable from the nested scope hides the one from the outer scope. The Dart language specification says that the parameters for a function (or method) are declared in a "parameter" scope and that the body of the function is a nested scope. So by definition it is valid to hide parameters by declaring a variable with the same name.

We could, of course, add a lint to prevent hiding parameter names, but I suspect that there would be too many false positives for it to be useful to very many people.

[DrRuhe]

void foo(int tmp) {
  tmp++; // Local variable 'tmp' can't be referenced before it is declared.
  tmp++; // Local variable 'tmp' can't be referenced before it is declared.

  var tmp = 5;
  tmp++;
}

When redeclaring variables is allowed, why does this cause errors?

Could this be solved by detecting if a variable is declared twice in the same scope?

Edit:
Thought about it a bit more:

When redeclaring variables is allowed, why does this cause errors?

Probably because inside a given scope there can only be a single declaration for a variable that can be used.
Therefore since the declaration happens afterwards it throws the errors, since the declaration of the outer scope cannot be used for only half of the function body.

Could this be solved by detecting if a variable is declared twice in the same scope?

Well I think I overread this part:

The Dart language specification says that the parameters for a function (or method) are declared in a "parameter" scope and that the body of the function is a nested scope.

[eernstg]

When redeclaring variables is allowed, why does this cause errors?

The local variable var tmp is in scope in the entire body of foo, and the parameter int tmp is in the enclosing scope. So tmp++ at the beginning of the body of foo is a reference to the local variable (not the parameter), and it is an error to use a local variable before its declaration.

Apart from the fact that this is just the way the scopes are structured, you could also argue that it would be confusing to allow the first tmp++ to refer to the parameter and the last tmp++ to refer to the local variable, because anyone who reads the code and doesn't notice the var tmp in the middle would be quite likely to misread that last tmp++ and think that they all modify the parameter.

[DrRuhe]

Could the error message be adjusted to include a reference to the place where it was declared?

Going from this message: Local variable 'tmp' can't be referenced before it is declared. to Local variable 'tmp' can't be referenced before it is declared on line X. would be nice.

As a bit of context, I stumbled upon this when refactoring some methods and I copied part of one method into another method. Since the destination already had the variable declared as a parameter and the pasted part contained the declaration it complained. I wondered why the variable was marked as undeclared, since the function parameter declared it, not realizing the problem was the redeclaration in the pasted part.

I only found the cause because a vscode extension had a reference to it in the menu that appears when hovering over the error, but I believe that is not part of the linter.

[eernstg]

Considering this as a request for a more informative error message from the analyzer, it should be moved to the SDK repo. Doing that now.

[eernstg]

As implied above, the error message from the analyzer does not mention the location of the declaration of the local variable in the following example, and it might be helpful to include that information:

void main(List<String> args) {
  args.length;
  var args;
}

The analyzer does not mention the declaration of args as a local variable, but the common front end reports the error with that information (and in the other "direction"):

$ dartanalyzer n017.dart
Analyzing n017.dart...
  error • Local variable 'args' can't be referenced before it is declared. • n017.dart:2:3 • referenced_before_declaration
1 error found.
$ dart n017.dart
n017.dart:3:7: Error: Can't declare 'args' because it was already used in this scope.
  var args;
      ^^^^
n017.dart:2:3: Context: Previous use of 'args'.
  args.length;
  ^^^^

[DrRuhe]

Can't declare 'args' because it was already used in this scope.

Would it be possible to make a linting rule out of this?

[bwilkerson]

... the error message from the analyzer does not mention the location of the declaration ...

Actually, the analyzer already produces this information, but as a "context message". Unfortunately it isn't displayed unless you include the --verbose flag on the command line. Sounds like we might want to rethink not displaying that information. (The advantage of this approach is that in an IDE the context message is clickable, so users can navigate to the location.)

Would it be possible to make a linting rule out of this?

Yes, but as I said above, "I suspect that there would be too many false positives for it to be useful to very many people."

[DrRuhe]

Does this still apply though? The only time this lint/error message should be displayed is when declaring a variable already used in this scope, which causes an error anyway. I don't see where there could be any false positives then.

[bwilkerson]

That's valid. I was thinking earlier about a less constrained lint.

[srawlins]

Sorry for the delay, what is the request for the lint rule? Is it that, given this code:

void foo(int tmp) {
  tmp++; // error here

  var tmp = 5;
}

we would continue to error on tmp++, and also report a lint on var temp = 5;? I don't think that's what we want to do. That is the purpose of the context messages: to point out a related bit of code, maybe the root cause of the issue.

I think the solution to the UX issue here is just to add more text to the message like, "Local variable 'tmp' can't be referenced before it is declared on line X." or "note that declaration shadows another variable in this scope."