dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.19k stars 1.57k forks source link

`dart fix` should remove `const`s when they err just as it adds them when needed #49818

Open satvikpendem opened 2 years ago

satvikpendem commented 2 years ago

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 commented 2 years ago

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

satvikpendem commented 2 years ago
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 commented 2 years ago

Perfect, thank you!

bwilkerson commented 2 years ago

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()];
github-actions[bot] commented 1 year ago

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 commented 1 year ago

(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 commented 1 year ago

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.

asashour commented 1 year ago

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