Pasting code with unknown color symbol prefixes it with colorScheme.

[lukehutch]

Describe the bug

Select code like this:

Paste into another class, where the field Color backgroundColor is not present:

The onTap and icon fields are pasted as-is. However, backgroundColor gets transformed into colorScheme.background.

This is unexpected, and frankly weird...

It may have something to do with the special rendering of colors in Dart-Code, with a color swatch thumbnail.

Please complete the following information:

• Operating System and version: Linux
• VS Code version: latest
• Dart extension version: latest
• Dart/Flutter SDK version: latest
• Target device (if the issue relates to Flutter debugging):

[DanTup]

Well that is odd. I was expecting to not be able to reproduce this, but if I create a new file with:

import 'package:flutter/material.dart';

var a = ElevatedButton.styleFrom(
  backgroundColor: backgroundColor,
);

And then press Save (which triggers fix-all-on-save), then I see the same thing:

import 'package:flutter/material.dart';

var a = ElevatedButton.styleFrom(
  backgroundColor: colorScheme.background,
);

I think this might be coming from a Flutter fix - there are a few that seems to migrate to "colorScheme.background" (such as flutter/flutter#110162), but at a glance, they all seem to have filters that I would expect to stop them firing here. I'll try to narrow it down.

[DanTup]

Looks like it's coming from this:

  # Changes made in https://github.com/flutter/flutter/pull/110162
  - title: "Migrate to 'ColorScheme.background'"
    date: 2022-08-24
    element:
      uris: [ 'material.dart' ]
      field: 'backgroundColor'
      inClass: 'ThemeData'
    changes:
      - kind: 'rename'
        newName: 'colorScheme.foo'

@bwilkerson does this seem like a bug in the data driven fixes? The fix has inClass: 'ThemeData' but there's no ThemeData involved here - there's an unresolved variable called backgroundColor and it's being substituted (see my comment above for a repo - that's the only thing in the file).

[bwilkerson]

The description makes it sounds like this is happening on 'paste', but I'm guessing that this is happening on 'save'. Even so, it's not clear to me that we want this kind of fix to run on save, given that the user might be mid-thought and have a different kind of fix in mind.

That said, yes, it seems like this is a bug. We know that the backgroundColor the transformation is referring to is in ThemeData, but there's (presumably) no way the reference can be referring to that field, so we shouldn't be transforming the reference.

[lukehutch]

backgroundColor is just a Color constant, so no, it doesn't refer to ThemeData.

I tried to replicate it right now to see if it was triggered on paste (as I vaguely remember it), or only on save (as @bwilkerson assumes). Right now I can't replicate this, for some reason, even though I have seen it happen several times. I hit save constantly (old Eclipse habit, since it saves a history marker in Eclipse every time you do that), so I may not have remembered that it was save and not paste that did it.

[bwilkerson]

As far as I know, we're not able to perform a code change like that on 'paste', hence my guess. But however it's happening I think its a bug that we're making the transformation in this case, and maybe that we're applying transformation at this time even if it were only performing correct transformations.

[DanTup]

As far as I know, we're not able to perform a code change like that on 'paste', hence my guess.

This is correct - there is no paste event in VS Code (well, it's still a proposed API so we can't use it yet). Though you can have auto-save enabled, as well as fix-all-on-save, so you could still see fixes run "just after" pasting. When reproducing this, I saw the change and thought I had also not saved.. however I then couldn't reproduce it without saving, so also assume I must have done it out of habit.

Even so, it's not clear to me that we want this kind of fix to run on save

Do you mean this fix specifically, or do you think all data-driven fixes should be excluded when fixes run on-save?

If it's going to do things like this, I'd agree - but if this issue was fixed, I'm less sure. If the user has chosen to fix-all-on-save, any fixes that are excluded are a potential source of confusion ("running Fix All fixes this, but setting fix-all-on-save does not"). Are these kinds of fixes (when working correctly) likely to be (or potentially) disruptive?

[bwilkerson]

I was probably reacting more to the buggy behavior than to the behavior is ought to have once the bug is fixed.

[DanTup]

@bwilkerson got it, thanks!

I'm not very familiar with this code, but it seems to go wrong here:

https://github.com/dart-lang/sdk/blob/3c4d4ad450a472ad9d11635a81d9ba278896c576/pkg/analysis_server/lib/src/services/correction/fix/data_driven/element_matcher.dart#L68-L75

// The node has fewer components, which can happen, for example, when we
// can't figure out the class that used to define a field. We treat the
// missing components as wildcards and match the rest.

elementComponents is ["backgroundColor", "ThemeData"] and components is ["backgroundColor"]`. Since there's no class in components, it only compares the first items and they match. Disallowing these mismatched lengths fixes this issue, but (unsurprisingly) breaks a lot of other tests. It's not clear to me what a correct fix would be.

I'm not sure of the impact of this bug - if it's likely low and the fix is complex, I could also commit a test marked as failing for later.

Btw, while looking at this I initially found this function which I think is unused (perhaps superseded by the ElementMatcher class linked above?).

[bwilkerson]

The code in element matcher is there so that if we find code of the form data.backgroundColor and don't know what data is we can still update the use site. I think the right fix is to notice when there is no receiver (data), which means that the identifier can only refer to a local variable or a field in the hierarchy. For the second case we should check to see whether the class containing the identifier (assuming it isn't in a function or static method) is a subtype of the class defining the element that was transformed (such as ThemeData). If it is a subtype, then the transform still makes sense; if not, then it doesn't.

I don't know whether the function is being used; I'd have to search for references to it. If it is unused, then it would be good to get rid of it.

@keertip

[keertip]

I will take a look.

[keertip]

I have landed a fix for this. Will be in the next release of the SDK.

[DanTup]

Great, thanks!

Closing this as fixed by dart-lang/sdk@c452dfc.

[lukehutch]

Thanks @keertip!