Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dart fix should remove consts when they err just as it adds them when needed #49818

Open
satvikpendem opened this issue Aug 25, 2022 · 8 comments
Labels
analyzer-quick-fix area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@satvikpendem
Copy link

dart fix automatically adds const which is great, but if we change some value to be non const, such as in a Flutter widget tree, we get errors. In my opinion, dart fix should remove those consts because it knows where the error is already.

Dart SDK version: 2.17.6 (stable) (Tue Jul 12 12:54:37 2022 +0200) on "windows_x64"

@bwilkerson
Copy link
Member

Can you provide a minimal reproduction case in which the diagnostic is reported but dart fix fails to remove the const?

@bwilkerson bwilkerson added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-bulk-fix labels Aug 25, 2022
@satvikpendem
Copy link
Author

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      title: 'Flutter Demo',
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({Key? key, required this.title}) : super(key: key);

  final String title;

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  final int counter = 0;

  @override
  Widget build(BuildContext context) {
    return const Scaffold(
      body: Center(
        child: Text(
          'Counter: $counter',
        ),
      ),
    );
  }
}

image

# In the terminal
❯ dart fix --dry-run
Computing fixes in testproj (dry run)...
Nothing to fix!

@bwilkerson
Copy link
Member

Perfect, thank you!

@bwilkerson
Copy link
Member

For anyone looking at adding such a fix, I'll point out that while the posted example is straightforward (as requested), the semantics of the fix are a bit harder. We'd want to remove const from the place that's causing the code to be in a const context while adding it in places that were previously implicitly const but aren't when the outer const is removed. For example,

class A {
  const A();
}

List<Object> f(int i) {
  return const [A(), 'i = $i'];
}

should be converted to

class A {
  const A();
}

List<Object> f(int i) {
  return [const A(), 'i = $i'];
}

so that the semantics are preserved as much as possible.

Also, the fix has to deal with places where const has to be replaced with final and not just removed, such as

class A {
  const A();
}

class B {}

const x = [A(), B()];

@srawlins srawlins added the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Aug 26, 2022
@github-actions
Copy link

Without additional information we're not able to resolve this issue, so it will be closed at this time. You're still free to add more info and respond to any questions above, though. We'll re-open the case if you do. Thanks for your contribution!

@devoncarew
Copy link
Member

(Note: we recently added a 'no response' bot to our repo; it may have closed existing issues with the needs-info label incorrectly. If you think your issue was effected, please feel free to reopen the issue)

@satvikpendem
Copy link
Author

I don't believe this issue should be closed, and I can't reopen the issue myself, I don't see the option on GitHub.

@github-actions github-actions bot removed the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Nov 14, 2022
@github-actions github-actions bot reopened this Nov 14, 2022
@pq pq added the P3 A lower priority bug or feature request label Nov 14, 2022
@asashour
Copy link
Contributor

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

There are possibly other diagnostics to be associated with the RemoteConst, but that's not part of the CL scope.

Please note that SPREAD_EXPRESSION_FROM_DEFERRED_LIBRARY should be renamed NON_CONSTANT_MAP_ELEMENT_FROM_DEFERRED_LIBRARY, and
SET_ELEMENT_FROM_DEFERRED_LIBRARY should be NON_CONSTANT_SET_ELEMENT_FROM_DEFERRED_LIBRARY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-quick-fix area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants