sdk

sdk copied to clipboard

Working on https://github.com/flutter/flutter/pull/111711, I was trying to write a fix for it, but no transforms are being applied.

The original fix modified some parameters, but I found just simply trying to replace the class constructor was not working. Simplified fix for debugging:

- title: "Migrate to 'RenderConstraintsTransformBox'"
    date: 2021-03-24
    element:
      uris: [ 'rendering.dart' ]
      constructor: ''
      inClass: 'RenderUnconstrainedBox'
    changes:
      - kind: 'replacedBy'
        newElement:
          uris: ['rendering.dart']
          constructor: ''
          inClass: 'RenderConstraintsTransformBox'

Unmigrated code:

RenderUnconstrainedBox renderBox = RenderUnconstrainedBox(
    alignment: Alignment.center,
    textDirection: TextDirection.ltr,
  );

Expected transform:

RenderConstraintsTransformBox renderBox = RenderConstraintsTransformBox(
    alignment: Alignment.center,
    textDirection: TextDirection.ltr,
  );

Sep 19 '22 23:09 Piinks

(I am not sure about correct area-* label for dart fix. Let me know if analyzer is the wrong one @srawlins)

Sep 20 '22 08:09 mraleph

I saw an email response to this issue, but it does not appear here. It mentioned a test case that currently shows this type of fix working. I am not sure if this is an exception or edge case, but the test case above does not work. I also came across another replacedBy constructor not working today in https://github.com/flutter/flutter/pull/108052

Are we using it wrong? I do not see a difference in the tested rule versus the rule that is not working. 😞

cc @bwilkerson

Sep 20 '22 21:09 Piinks

I saw the response in email too. The response was from @asashour. It might have been deleted, but I don't know how to get history for the issue to confirm that. I can paste it back in if the deletion wasn't intentional.

I don't see any problem with the code above, except that as Ahmed saw in his test case, the transform is specifying a replacement for a constructor, not for the type, so I wouldn't expect the type annotation before renderBox to be replaced.

I'd need to do further investigating to understand more about the problem. But one question you might be able to answer: does the code simply not get transformed at all, or is it transformed but not in the way you expected?

Sep 20 '22 22:09 bwilkerson

does the code simply not get transformed at all, or is it transformed but not in the way you expected?

Thanks @bwilkerson! There is no transformation at all in the example here and in https://github.com/flutter/flutter/pull/108052

Sep 20 '22 22:09 Piinks

Update: the test case below passes when the error offset is changed.

Sorry for the disturbance. After writing the first comment, and investigating further, there are some results which made the first comment not to the point, that's why it was deleted, to implicitly say: please ignore it.

The test case that doesn't trigger anything is far below.

However, there is some transformation done if the deprecation is not at Old class, but at the constructor, e.g.

class Old {
  @deprecated
  Old({String? a, String b,});
}

which was the result of the deleted comment.

The current test cases put the @deprecated at the constructor, possibly not detecting the case in question.

Having a minimal complete case is crucial (at least for me), to quickly see see how things work, and using Flutter code may impose a challenge, specially when someone is new to the area.

I would dig a little more based on my current understanding. Thanks.

  Future<void> test_constructor() async {
    setPackageContent('''
@deprecated
class Old {
  Old({String? a, String b,});
}
class New {
  New({String? a, String b,});
}
''');
    addPackageDataFile('''
version: 1
transforms:
  - title: 'Migrate constructor'
    date: 2022-05-12
    element:
      uris: ['$importUri']
      constructor: ''
      inClass: 'Old'
    changes:
      - kind: 'replacedBy'
        newElement:
          uris: ['$importUri']
          constructor: ''
          inClass: 'New'
''');
    await resolveTestCode('''
import '$importUri';

f() {
  Old abc = Old(
    a: 'a',
    b: 'b',
  );
  return abc;
}
''');
    await assertHasFix('''
import '$importUri';

f() {
  New abc = New(
    a: 'a',
    b: 'b',
  );
  return abc;
}
''', errorFilter: (error) => error.offset == 41);
  }

Sep 20 '22 22:09 asashour

The reason seems to be that Flutter disables deprecation errors, and so dart fix doesn't find any relevant fixes.

For the time being, this can be handled by temporarily removing the disablement before running the fix. On the other hand, the data driven fix could be changed to consider all lints/errors (even if disabled).

I hope this clarifies, since potentially more issues would arise based on that.

Sep 22 '22 11:09 asashour

The reason seems to be that Flutter disables deprecation errors, and so dart fix doesn't find any relevant fixes.

@asashour this is not the case. I was not trying to fix a deprecation, as I was deleting the deprecated code when I wrote the fix. RenderUnconstrainedBox did not exist anymore, and dart fix still was not fixing the code.

Also, we test the fixes in an environment different from the analysis options you fixed, specifically for the reasons you mentioned:

https://github.com/flutter/flutter/blob/master/packages/flutter/test_fixes/analysis_options.yaml

Sep 22 '22 16:09 Piinks

For reference, here is where the API was being deleted and the transformation not working: https://github.com/flutter/flutter/pull/111711

Sep 22 '22 16:09 Piinks

https://dart-review.googlesource.com/c/sdk/+/260761

Thanks for the explanation.

On looking on the PR, I thought that the fix was applied before the commit, so the "old" class and constructor were existing and are deprecated, and this was already supported (if the deprecation error is enabled)

The proposed CL lets replacedBy handle the case when the "old" class is not existing.

Sep 23 '22 07:09 asashour

I think I still have the same issue, I'm on:

Dart SDK version: 2.19.0-288.0.dev (dev) (Thu Oct 6 20:22:42 2022 -0700) on "macos_x64"

If I add this rule to fixes:

# Changes made in https://github.com/flutter/flutter/pull/108052
  - title: 'Replace with ShapeDecoration'
    date: 2022-10-07
    element:
      uris: [ 'painting.dart', 'rendering.dart', 'widgets.dart', 'material.dart', 'cupertino.dart' ]
      inClass: 'BoxDecoration'
      constructor: ''
    oneOf:
    - if: "shape == 'BoxShape.circle'"
      changes:
        - kind: 'replacedBy'
          newElement:
            uris: [ 'painting.dart', 'rendering.dart', 'widgets.dart', 'material.dart', 'cupertino.dart' ]
            inClass: 'ShapeDecoration'
            constructor: ''
        - kind: 'addParameter'
          index: 1
          name: 'shape'
          style: optional_named
          argumentValue:
            expression: '{% CircleBorder %}()'
            requiredIf: "shape == 'BoxShape.circle'"
            variables:
              CircleBorder:
                kind: 'import'
                uris: [ 'painting.dart', 'rendering.dart', 'widgets.dart', 'material.dart', 'cupertino.dart' ]
                name: 'CircleBorder'
        - kind: 'removeParameter'
          name: 'boxShadow'
        - kind: 'addParameter'
          index: 4
          name: 'shadows'
          style: optional_named
          argumentValue:
            expression: '{% boxShadow %}'
            requiredIf: "boxShadow != ''"
        - kind: 'removeParameter'
          name: 'shape'
        - kind: 'removeParameter'
          name: 'borderRadius'
    variables:
      shape:
        kind: 'fragment'
        value: 'arguments[shape]'
      boxShadow:
        kind: 'fragment'
        value: 'arguments[boxShadow]'

and test with:

import 'package:flutter/painting.dart';

void main() {
  BoxDecoration(
    shape: BoxShape.circle,
    color: Color(0xFFFF0000),
    boxShadow: [],
  );
}

the output is not ShapeDecoration. The output is the same.

Oct 07 '22 18:10 bernaferrari

@bernaferrari are you applying this rule in a branch where shape is deprecated?

Oct 07 '22 18:10 Piinks

Yes. At this point, I guess it is easier to just send the branch lol

The remove BoxShape.rectangle work. Only the replace-by fails.

Oct 07 '22 18:10 bernaferrari

It's possible that the fix has not made it into the latest dev branch and that you might need to use a newer version of the Dart SDK. I'm not sure how to verify that.

It's also possible that the fix missed a case.

Oct 07 '22 19:10 bwilkerson

I wish there were a simple way to test these fixes, maybe a playground of some sort. I can't even share with you "everything" (like the BoxDecoration class) because each thing is in a different place. The person that's going to test is going to spend more time than needed.

I think it is, because the commit was from 2 weeks ago and the dev commit is from yesterday, so I guess it has had enough time to be there. But I don't know.

Oct 07 '22 19:10 bernaferrari

The commit seems to be available since 2.19.0-262.0.dev (as shown in its tags).

The change was handling the case where the the old (BoxDecoration in this case) doesn't exist at all.

Having the BoxDecoration in the code (deprecated or not) is outside of the fix scope.

To find a way forward, a minimal test case would be really helpful, preferably outside Flutter code base (as it is big and is continuously changing).

Oct 07 '22 19:10 asashour

To make a minimal case:

1. Clone/fork flutter (packages/flutter is used below)
2. In Flutter, replace lib/fix_data.yaml with

version: 1
transforms:
  - title: 'Replace with C_new'
    date: 2022-10-07
    element:
      uris: [ 'abc.dart' ]
      inClass: 'C_old'
      constructor: ''
    changes:
      - kind: 'replacedBy'
        newElement:
          uris: [ 'abc.dart' ]
          inClass: 'C_new'
          constructor: ''

2. In Flutter lib, have abc.dart with:

class C_old {
  @deprecated
  C_old();
}

class C_new {
  C_new();
}

4. In another project, where it uses this cloned Flutter:

import 'package:flutter/abc.dart';

void f() {
  C_old();
}

5. In your IDE (IntelliJ with me), right click on the C_old in the other project, and you will have Replace with C_new where it is changed.

From this point, add to the fix_data.yaml, step by step, to reach your desired behavior, and when the minimal scenario it doesn't work, please post it here (or in a new issue).

Oct 07 '22 20:10 asashour

@bernaferrari your case works. Make sure you don't have the test class inside Flutter master project, because it ignores @deprecation, which is required to produce an error, which is required to trigger the data-driven fixes.

Steps:

1. Using Flutter master
2. Deprecate BoxDecoration class
3. Replace packages/flutter/lib/fix_data.yaml with:

version: 1
transforms:
  - title: 'Replace with ShapeDecoration'
    date: 2022-10-07
    element:
      uris: [ 'painting.dart', 'rendering.dart', 'widgets.dart', 'material.dart', 'cupertino.dart' ]
      inClass: 'BoxDecoration'
      constructor: ''
    oneOf:
      - if: "shape == 'BoxShape.circle'"
        changes:
          - kind: 'replacedBy'
            newElement:
              uris: [ 'painting.dart', 'rendering.dart', 'widgets.dart', 'material.dart', 'cupertino.dart' ]
              inClass: 'ShapeDecoration'
              constructor: ''
          - kind: 'addParameter'
            index: 1
            name: 'shape'
            style: optional_named
            argumentValue:
              expression: '{% CircleBorder %}()'
              requiredIf: "shape == 'BoxShape.circle'"
              variables:
                CircleBorder:
                  kind: 'import'
                  uris: [ 'painting.dart', 'rendering.dart', 'widgets.dart', 'material.dart', 'cupertino.dart' ]
                  name: 'CircleBorder'
          - kind: 'removeParameter'
            name: 'boxShadow'
          - kind: 'addParameter'
            index: 4
            name: 'shadows'
            style: optional_named
            argumentValue:
              expression: '{% boxShadow %}'
              requiredIf: "boxShadow != ''"
          - kind: 'removeParameter'
            name: 'shape'
          - kind: 'removeParameter'
            name: 'borderRadius'
    variables:
      shape:
        kind: 'fragment'
        value: 'arguments[shape]'
      boxShadow:
        kind: 'fragment'
        value: 'arguments[boxShadow]'

4. With the below on an external project:

import 'package:flutter/painting.dart';

void main() {
  BoxDecoration(
    shape: BoxShape.circle,
    color: Color(0xFFFF0000),
    boxShadow: [],
  );
}

It is converted to:

  ShapeDecoration(
    shape: CircleBorder(),
    color: Color(0xFFFF0000), shadows: [],
  );

Just check that you have a deprecation warning, something like the below (const was added to not have many warnings, but it doesn't have an effect):

Oct 08 '22 07:10 asashour

I don't want to deprecate BoxDecoration, I want to deprecate shape property and replace BoxDecoration with ShapeDecoration when shape is circle.

Oct 08 '22 12:10 bernaferrari

https://dart-review.googlesource.com/c/sdk/+/263260

When a parameter is deprecated, replacedBy was changing the argument instead of changing the constructor.

Oct 08 '22 17:10 asashour

That was fast. Thanks a lot!

Oct 08 '22 17:10 bernaferrari

Sorry that my review of the CL has not been as fast.

If I'm understanding the most recent use case (I think there's more than one that's been discussed in this issue), the desire is to have the use of a deprecated (optional) argument in a constructor invocation cause the whole constructor invocation to be replaced by an invocation of a different constructor.

If that's the case then I have two concerns.

The first is that the proposed fix isn't as clearly the right path forward for the user as most of the fixes we currently support. I could imagine a user wanting to delete the argument rather than to invoke a different constructor on a different class. If it is possible that this fix isn't the only valid fix, then it isn't clear to me that this kind of fix is something we want to enable for bulk application where a user might not notice that the incorrect fix had been applied. That violates one of the general design principles we use for bulk fixes.

The second is that it seems wrong to say that every deprecated parameter in a constructor should cause the tool to look for a change associated with the constructor. That seems to me to be too general and error prone in the sense that we might start applying changes to constructors that the fix data author didn't intend or expect would be applied because the trigger isn't obvious.

Oct 25 '22 16:10 bwilkerson

I think you raise some really valid points @bwilkerson. I agree that this is the type of migration we may not want to support.

@bernaferrari have you drafted the migration guide for your change yet? It could be that the change you are making has multiple migration decisions that are up to the user. Having a migration guide on the website is how we best support users migrating to those changes. That may be the right course of action for your PR, and not include a data driven fix.

Oct 27 '22 19:10 Piinks

No, but it is pretty simple. Is there a file I should propose the change? I don't know where I should write it.

Oct 28 '22 17:10 bernaferrari

Happy to help! Let's sync up on your PR. @bwilkerson do you feel that we should close this issue then?

Oct 28 '22 17:10 Piinks

If there's no outstanding bugs or requests for us to respond to, then closing it is fine. We can always re-open it if there's something we missed.

Oct 28 '22 17:10 bwilkerson