"Find all references" does not treat field formal parameters as references to fields

Describe the bug

"Find all references" seems to discriminate between this. parameters and the fields they map to. I don't think there should be a distinction.

To Reproduce

Create these two classes:

class X {
  int x;  // (1)
  X({required this.x});  // (2)
}

class Y {
  void foo() {
    X(x: 1);  // (3)
  }
}

Actual behavior

The numbers (1), (2), and (3) infer positioning the cursor at x on the correspondingly numbered line, and then right-clicking and selecting "Find all references":

"Find all references" at (1) finds only (1) and (2).
"Find all references" at (2) finds only (1) and (3).
"Find all references" at (3) finds only (2) and (3).

Expected behavior

"Find all references" at any of (1), (2), or (3) should find all three.

(Dart 3.3.0 on Linux)

Not sure but I think it is correct behavior and it should like that? It helpful when understand (1) as local, (2) and (3) as global.

@bwilkerson

"Find all references" at any of (1), (2), or (3) should find all three.

I'm not convinced that's the right behaviour. Find references should find references to the selected item. The locations (1) and (2) are not references to the argument (3).

Perhaps there's an argument to be made that find-references on an argument should find references to the parameter. However, I still don't think that means (1) should be returned as a reference to (3). It's a field and not a reference to anything. (@bwilkerson interested in your thoughts).

That said, I see some odd behaviour testing this. I was checking whether we usually treat arguments like parameter when finding references and discovered that with:

void g({int? aaa}) {
  // g(aaa: 1);
}

void g2() {
  g(aaa: 1); // Find refs on aaa here
}

Find references on the marked parameter does not find anything. However, if I uncomment the commented line, and then repeat the search (again, on the marked one - not on the uncommented line) I then get three results.

This might be partly confused because VS Code issues multiple requests when you search for references/definitions - sometimes if it doesn't get any results, it will search the opposite (for convenience so you can just Ctrl+Click on a definition to get references), but I'll need to do some more testing.

The UX around this is tricky.

From the point of view of the spec, an initializing formal parameter (this.x) doesn't refer to the field at all, it defines a formal parameter whose name is x that is only visible in the initializer list (implying, not in the body of the constructor). It behaves as if there were an assignment of the form this.x = x in the initializer list.

So, from a spec perspective, finding references at (1) should return (1) and finding references at either (2) or (3) should return both (2) and (3).

But that's not what most users would expect, nor, I suspect, find useful.

Based on the spec, I can rationalize some of the current behavior in the following way:

When you find references at (1) we're returning references to the field, which includes both the declaration of the field and the hidden assignment to the field in the initializer list. Given that the assignment isn't visible in the source code we're showing it by returning the initializing formal parameter that introduces the assignment.
When you find references at (3) we're returning references to the name of the parameter, which includes (2) and (3). (Note that the name of the parameter and the parameter itself are two different things.)

I don't have a rationalization for the current behavior of find references at (2). It seems like we should either treat it as a reference to the parameter and return (2) and (3), or, more likely, treat it as a reference to both the field and the parameter and return all three.

That's assuming that we don't want to just treat the field and the parameter as being essentially the same thing. I might argue against doing that (treating them as the same thing) because I think it might be useful for users to know that an explicit use of x in the initializer list isn't a reference to the field and that a use of x in the constructor body isn't a reference to the parameter. On the other hand, if you rename x in any of those three locations we'll rename it in all three of those locations in order to preserve semantics.

I'll also note that if you click on x in any of those three positions we mark all 3 locations as referring to the same thing. If nothing else we have an inconsistency that probably leads to some of the confusion, and we ought to also consider making occurrence marking consistent with the solution we choose here.

for convenience so you can just Ctrl+Click on a definition to get references

Thanks for the pro-tip :-)

That's assuming that we don't want to just treat the field and the parameter as being essentially the same thing.

Honestly because of the this.x notation, this is the only thing that makes sense to me. Outside of this (invisible) code execution equivalent to this.x = x, literally everywhere else in the program appears to the uninitiated observer to be writing to the field (in constructor calls) or reading from the field (if the field is public).

I might argue against doing that (treating them as the same thing) because I think it might be useful for users to know that an explicit use of x in the initializer list isn't a reference to the field and that a use of x in the constructor body isn't a reference to the parameter.

I don't know how it's useful to know the distinction under the hood. All I care about is finding everything, everywhere, that is a setter or a getter of the field that appears (to me) to be bound to the constructor parameter.

On the other hand, if you rename x in any of those three locations we'll rename it in all three of those locations in order to preserve semantics.

I'll also note that if you click on x in any of those three positions we mark all 3 locations as referring to the same thing. If nothing else we have an inconsistency that probably leads to some of the confusion, and we ought to also consider making occurrence marking consistent with the solution we choose here.

Those are two excellent points, and they further obscure the distinction between a this.x parameter and the field itself. In hindsight these are behaviors that created the expectation in my mind that led to me filing this bug report. Honestly these two behaviors feel the most natural to me, and they match my expectations.

I wouldn't want either behavior changed, personally, because both are intuitive and useful. Ultimately intuitiveness and usefulness trump having a deep understanding of the nuances of semantics.

I don't know how it's useful to know the distinction under the hood.

I could easily see a user finding code like the following to be confusing:

void main() {
  C(f: 2);
}

class C {
  int f;

  C({required this.f}) {
    initialize();
    print(f);
  }

  void initialize() {
    f++;
  }
}

It looks (to me at least) like it ought to print the value passed to the constructor, but it doesn't, it prints the value of the field. Of course, it's only a problem with non-final fields, and even then it probably doesn't come up much. I'm not saying that we ought to design for such a corner case, just that it exists.

Ultimately intuitiveness and usefulness trump having a deep understanding of the nuances of semantics.

I generally agree. I think that it's worth considering those cases where the "nuances of semantics" are hidden by the UX in such a way that it makes it hard for users to understand what the code is doing, but other than that you're right. I think I'm convinced by this discussion that find references should behave the same as mark occurrences and rename.

Maybe it would make sense to have a lint that flags uses of fields associated with initializing formal parameters in the body of a constructor. That's really where the potential for confusion lies, and I suspect that it's rarely something that users need to do.

I think I'm convinced by this discussion that find references should behave the same as mark occurrences and rename.

I've implemented what I think we're agreeing here at https://dart-review.googlesource.com/c/sdk/+/364120. Finding references on a FieldFormalParameter will work the same as finding them on the field (because we just map the parameter onto its field), and finding references on a field will include any FieldFormalParameters in the hierarchy for that field.

Thanks. I ran into a related issue yesterday... I tried renaming a constructor parameter from a reference, and a bunch of code broke because the parameter was set in a subclass via super.fieldName, which seemed to stop the rename in its tracks. I can't remember the exact details and didn't have time to stop and create a repro case for this, but would your change fix this, based on this vague description?

I would not expect my change above to affect this - it only changes Find References. If you can reproduce this, please do file an issue (at https://github.com/dart-lang/sdk).

Will keep this open until the change above is agreed/landed.

@DanTup

@DanTup sorry, I clicked the wrong button, I didn't mean to close this!

Here is a repro for the issue in my previous comment, filed in dart-lang:

dart-lang/sdk#55552

[analysis_server] Treat field formal parameters and fields equally wh…

Fixed by dart-lang/sdk@bf7f517. The fix is in the analysis server, so ships in a future SDK release.

Navigation Menu

Repository navigation

"Find all references" does not treat field formal parameters as references to fields #4868

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

"Find all references" does not treat field formal parameters as references to fields #4868

Description

Activity

quyenvsp commented on Dec 1, 2023

DanTup commented on Dec 1, 2023

bwilkerson commented on Dec 1, 2023

lukehutch commented on Dec 1, 2023

bwilkerson commented on Dec 1, 2023

DanTup commented on Apr 23, 2024

lukehutch commented on Apr 23, 2024

DanTup commented on Apr 23, 2024

lukehutch commented on Apr 24, 2024

DanTup commented on Jun 13, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions