Unused local elements warnings (unused local variable, unused element,

[srawlins]

[srawlins]

I started work on this and have stumbled on two questions:

1. Should we report an unused private representation field on extension types?

So code like:

extension type ET(int _it) {}

If yes, I guess we already should have, but we don't. There are some extension types like this in the CFE sources (cc @johnniwinther @stereotype441). Is there any reason to have an extension with an unused representation field? If there is, should we recommend making it public? Should we recommend naming it _? A field declared with the name _ is not a wildcard; it can be referenced. So normally we would still report an unused field, named _, but we could make an exception for extension types?

2. Should we report an unused instance-variable-declaring parameter in a primary constructor named _?

So code like:

class C(var int _x, var int _);

We don't report parameters in general named _ as unused because, well that was the whole point of wildcards. But an instance-variable-declaring parameter in a primary constructor declares both a parameter (which I assume would be a wildcard parameter) and an instance variable. And if the instance variable is unused then... I think we should report it.

CC @bwilkerson and @eernstg for thoughts.

[bwilkerson]

1. Should we report an unused private representation field on extension types?

No. It's my understanding that extension types are required to have exactly one representation typed field, so I don't think we should warn about this case. (We should consider letting users know if it could be a wildcard, though.)

2. Should we report an unused instance-variable-declaring parameter in a primary constructor named _?

Yes. I think we should report a declaring parameter that introduces a field that isn't being referenced. But we'd be reporting the field, not the parameter, so the problem message probably needs to be different. Certainly the correction message should be different, because one solution is to remove the var or final.

[srawlins]

(We should consider letting users know if it could be a wildcard, though.)

Fields cannot be wildcard variables. I don't know if naming the representation field as _ makes anything clearer. I think it would lead to confusion as it looks like it might be a wildcard.

[eernstg]

Should we report an unused private representation field on extension types?

I think we might recommend renaming it to _.

It is not possible for the representation variable of an extension type E to be unused if any object is ever accessed with static type E; or for a generic extension type E<X1 .. Xk> if any object has a type of the form E<T1 .. Tk>, for any actual type arguments T1 .. Tk. The storage used to hold the reference of type E or E<T1 .. Tk> is the storage that holds the representation variable as well.

The representation variable can also be initialized without access to its name, using expressions like myObject as E. Another example is the widespread use of extension types of the following form in interop code:

extension type HTMLLabelElement._(JSObject _) implements HTMLElement, JSObject {
  HTMLLabelElement() : _ = document.createElement('label');
  ... // External instance members
}

The primary constructor in HTMLLabelElement is not intended to be used (at all!), it just serves as the means to declare the representation variable. So the primary constructor is private. However, the representation variable is also not intended to be used directly, hence the wildcard parameter and the implicitly induced representation variable _.

The point is that the value of the representation variable is obtained by calling document.createElement, and all operations on the resulting object are intended to be performed by calling the external instance members. The external instance members will use the value of the representation variable implicitly. All in all, this extension type will do everything it is intended to do without ever accessing the representation variable explicitly. Clients also do not need to (indeed: should not) access the representation variable explicitly.

External constructors can take this approach even one step further:

extension type MountWidgetOptions._(JSObject _) implements JSObject {
  external MountWidgetOptions({
    ConsumerContainerElement container,
    String? loggingContext,
  });

  external ConsumerContainerElement container;
  external String? loggingContext;
}

Again, nobody needs to access the representation variable explicitly, and yet it is not unused.

I think this serves as a strong argument in favor of allowing the use of _ as the name of the representation variable, and not targeting it with any diagnostic messages. It would even have been OK if it had been treated as a wildcard, just like the primary constructor parameter. That is not the case technically, but I do expect the community to treat any declaration with the name _ pretty much as a wildcard, and that's fine.

If the primary constructor parameter of an extension type is a private name (such as _x), not a wildcard, and it is never accessed explicitly, then I'd recommend a diagnostic message that suggests a renaming to _.

It does not make sense to recommend that the representation variable itself shouldn't exist, but the code might be easier to read if, as a rule of thumb, representation variables are named _ when they are never accessed explicitly.

Should we report an unused instance-variable-declaring parameter in a primary constructor named _?

I think the answer would be no, not the parameter. But the implicitly induced instance variable could be reported, and it shouldn't make any difference whether it was declared explicitly or induced implicitly by a declaring parameter.

Here's the example again:

class C(var int _x, var int _);

The wildcarded parameter in the primary constructor isn't actually unused: there will be an actual argument for this parameter at every call site. Even in the case where it had been optional, the language semantics guarantees that the parameter is bound to a type correct value when the constructor is executed.

Next, the value of the parameter is used to initialize the corresponding implicitly induced instance variable _. So the parameter is being used.

The instance variable _ may or may not be unused (this would be a separate check which is identical for an explicitly declared instance variable and for an instance variable which is induced by a declaring parameter). I think it would make sense to emit a diagnostic suggesting that the parameter can be non-declaring, because the instance variable is just wasting space.

It could make sense to argue that anything named _ should be allowed to be unused without diagnostic intervention ("it looks like a wildcard, hence it is intentionally never used"). In this case we shouldn't report that an instance variable named _ is unused (again, no matter whether it's declared explicitly or induced implicitly). It could actually be accessed by external code based on an offset (so it's still true that the name is never used). However, I do think that's a bit too far-fetched.

In any case, I'd recommend that the implicitly induced instance variable named _ is reported according to exactly the same approach to "unused" as an explicitly declared instance variable with the same name. The difference is just that the advice would be to delete the explicit declaration respectively change the corresponding declaring parameter to be non-declaring.

[lrhn]

Agree.
Ad 2: Parameter is not unused, it's equivalent to an instance variable declaration and a this._varName, and a this._varName is never unused (or unnecessarily named). Only report if you would report a directly declared the instance member as unused.
Ad 1: Parameter and "field" are not unused, they're required to exist. That existing is a use. Whether to ask you to name it _ if the name isn't otherwise used ... that's probably fine, but not something I'd want to spend any time on. It's not that the name goes away like it does for local variables named _. There is no real difference between being _ and _it as an instance variable, they're both just private names.

[srawlins]

Only report if you would report a directly declared the instance member as unused.

Hmm, we do. We have an unused_field warning that reports when a private field is never read.

I think the answer would be no, not the parameter. But the implicitly induced instance variable could be reported

Well, we can't report one without the other. That is, we only report diagnostics at a given offset and length, within a given file. So if we want to report the field _x as unused in class C(final int _x);, then we'd report an "unused_field" warning at offset 18. Whether you consider that as "reporting the parameter" or "reporting the instance member" or both is maybe semantics.

[lrhn]

is maybe semantics.

That's all we do here 😉

But yes, the "unused instance variable" should be reported at the declaration of that variable, which is the same position as the declaration of the parameter. They're still two different semantic entities, even if they have the same syntactic origin.
It should not be an "unused parameter" warning.