Skip to content

enable lint unnecessary_const #19342

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.

Sign up for GitHub

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

Jump to bottom
Merged
a14n merged 1 commit into from
Jul 16, 2018
Merged

enable lint unnecessary_const #19342

a14n merged 1 commit into from
Jul 16, 2018

Conversation

@a14n
Copy link
Contributor

@a14n a14n commented Jul 13, 2018

No description provided.

@a14n
Copy link
Contributor Author

a14n commented Jul 13, 2018

@Hixie

@a14n a14n force-pushed the unnecessary_const branch from ea13993 to e90444c Compare July 13, 2018 19:59
@a14n a14n force-pushed the unnecessary_const branch from e90444c to cacb57a Compare July 16, 2018 06:07
@Hixie
Copy link
Contributor

Hixie commented Jul 16, 2018

LGTM

@a14n a14n merged commit cc1cf13 into flutter:master Jul 16, 2018
@a14n a14n deleted the unnecessary_const branch July 16, 2018 19:43
@a14n a14n mentioned this pull request Jul 16, 2018
Closed
4 tasks
@Hixie
Copy link
Contributor

Hixie commented Jul 16, 2018

This had a measurable impact (~10%) on the flutter_test_performance benchmark. We should probably revert this until we understand that issue. cc @a-siva @aam @cbracken

@Hixie
Copy link
Contributor

Hixie commented Jul 16, 2018

This also causes some compile errors:

2018-07-16T13:20:25.127363: stderr: [+3635 ms] Error: 'package:flutter/src/scheduler/priority.dart': error: line 20 pos 37: initializer is not a valid compile-time constant
stderr: [   +1 ms]   static const Priority animation = Priority._(100000);

Hixie added a commit that referenced this pull request Jul 16, 2018
@Hixie
Revert "enable lint unnecessary_const (#19342)"
18a9e36 
This reverts commit cc1cf13.
@Hixie Hixie mentioned this pull request Jul 16, 2018
Merged
Hixie added a commit that referenced this pull request Jul 16, 2018
@Hixie
Revert "enable lint unnecessary_const (#19342)" (#19423)
f630f90 
This reverts commit cc1cf13.
@Hixie
Copy link
Contributor

Hixie commented Jul 16, 2018

Reverted in #19423

@a14n
Copy link
Contributor Author

a14n commented Jul 16, 2018

This also causes some compile errors:

Where did you see the error ? Travis was ok https://travis-ci.org/flutter/flutter/builds/404310972

@Hixie
Copy link
Contributor

Hixie commented Jul 16, 2018

In our post-commit testing. Not sure why travis didn't catch it.

Here are some others:

2018-07-16T12:53:46.845310: stderr: [+3058 ms] Error: 'file:///Users/flutter/.cocoon/flutter/examples/hello_world/lib/main.dart': error: line 7 pos 43: expression is not a valid compile-time constant
stderr: [   +1 ms] void main() => runApp(const Center(child: Text('Hello, world!', textDirection: TextDirection.ltr)));

2018-07-16T13:02:02.944432: stdout: [ +196 ms] E/flutter (12568): [ERROR:topaz/lib/tonic/logging/dart_error.cc(16)] Unhandled exception:
2018-07-16T13:02:02.945051: stdout: [        ] E/flutter (12568): 'package:flutter/src/services/system_channels.dart': error: line 30: initializer is not a valid compile-time constant
stdout: [        ] E/flutter (12568):   static const MethodChannel navigation = MethodChannel(
stdout: [        ] E/flutter (12568):                                           ^
stdout: [        ] E/flutter (12568): 
2018-07-16T13:02:02.945230: stdout: [        ] E/flutter (12568): #0      new BindingBase (package:flutter/src/foundation/binding.dart:53)
stdout: [        ] E/flutter (12568): #1      new BindingBase&GestureBinding (package:flutter/src/gestures/binding.dart:19)
stdout: [        ] E/flutter (12568): #2      new BindingBase&GestureBinding&ServicesBinding (package:flutter/src/services/binding.dart:19)
2018-07-16T13:02:02.945364: stdout: [        ] E/flutter (12568): #3      new BindingBase&GestureBinding&ServicesBinding&SchedulerBinding (package:flutter/src/scheduler/binding.dart:189)
stdout: [        ] E/flutter (12568): #4      new BindingBase&GestureBinding&ServicesBinding&SchedulerBinding&PaintingBinding (package:flutter/src/painting/binding.dart:15)
2018-07-16T13:02:02.945639: stdout: [        ] E/flutter (12568): #5      new BindingBase&GestureBinding&ServicesBinding&SchedulerBinding&PaintingBinding&RendererBinding (package:flutter/src/rendering/binding.dart:24)
stdout: [        ] E/flutter (12568): #6      new BindingBase&GestureBinding&ServicesBinding&SchedulerBinding&PaintingBinding&RendererBinding&WidgetsBinding (package:flutter/src/widgets/binding.dart:235)
2018-07-16T13:02:02.946014: stdout: [        ] E/flutter (12568): #7      new WidgetsFlutterBinding (package:flutter/src/widgets/binding.dart:896)
stdout: [        ] E/flutter (12568): #8      WidgetsFlutterBinding.ensureInitialized (package:flutter/src/widgets/binding.dart:911)
stdout: [        ] E/flutter (12568): #9      runApp (package:flutter/src/widgets/binding.dart:703)
2018-07-16T13:02:02.946248: stdout: [        ] E/flutter (12568): #10     main (file:///private/var/folders/p7/qyw13f453r71jtt_d4dd8wgw0000gn/T/edited_flutter_gallery/lib/main.dart:20)
stdout: [        ] E/flutter (12568): #11     _startIsolate. (dart:isolate-patch/dart:isolate/isolate_patch.dart:279)
stdout: [        ] E/flutter (12568): #12     _RawReceivePortImpl._handleMessage (dart:isolate-patch/dart:isolate/isolate_patch.dart:165)

2018-07-16T13:02:13.907731: stdout: [   +2 ms] E/flutter (12568): [ERROR:topaz/lib/tonic/logging/dart_error.cc(16)] Unhandled exception:
stdout: [        ] E/flutter (12568): 'package:flutter/src/services/system_channels.dart': error: line 30 pos 43: initializer is not a valid compile-time constant
2018-07-16T13:02:13.908058: stdout: [        ] E/flutter (12568):   static const MethodChannel navigation = MethodChannel(
stdout: [        ] E/flutter (12568):                                           ^
stdout: [        ] E/flutter (12568): 
stdout: [        ] E/flutter (12568): #0      new BindingBase (package:flutter/src/foundation/binding.dart:53:5)
stdout: [        ] E/flutter (12568): #1      new BindingBase&GestureBinding (package:flutter/src/gestures/binding.dart:19:1)
stdout: [        ] E/flutter (12568): #2      new BindingBase&GestureBinding&ServicesBinding (package:flutter/src/services/binding.dart:19:1)
2018-07-16T13:02:13.908162: stdout: [        ] E/flutter (12568): #3      new BindingBase&GestureBinding&ServicesBinding&SchedulerBinding (package:flutter/src/scheduler/binding.dart:189:1)
stdout: [        ] E/flutter (12568): #4      new BindingBase&GestureBinding&ServicesBinding&SchedulerBinding&PaintingBinding (package:flutter/src/painting/binding.dart:15:1)
stdout: [        ] E/flutter (12568): #5      new BindingBase&GestureBinding&ServicesBinding&SchedulerBinding&PaintingBinding&RendererBinding (package:flutter/src/rendering/binding.dart:24:1)
2018-07-16T13:02:13.908251: stdout: [        ] E/flutter (12568): #6      new BindingBase&GestureBinding&ServicesBinding&SchedulerBinding&PaintingBinding&RendererBinding&WidgetsBinding (package:flutter/src/widgets/binding.dart:235:1)
stdout: [        ] E/flutter (12568): #7      new WidgetsFlutterBinding (package:flutter/src/widgets/binding.dart:896:1)
stdout: [        ] E/flutter (12568): #8      WidgetsFlutterBinding.ensureInitialized (package:flutter/src/widgets/binding.dart:911:11)
2018-07-16T13:02:13.908344: stdout: [        ] E/flutter (12568): #9      runApp (package:flutter/src/widgets/binding.dart:703:25)
2018-07-16T13:02:13.908444: stdout: [        ] E/flutter (12568): #10     main (/data/user/0/io.flutter.demo.gallery/cache/edited_flutter_galleryAHQWVA/edited_flutter_gallery/lib/main.dart:20:3)
2018-07-16T13:02:13.908529: stdout: [        ] E/flutter (12568): #11     _startIsolate. (dart:isolate-patch/dart:isolate/isolate_patch.dart:279)

@Hixie
Copy link
Contributor

Hixie commented Jul 16, 2018

These are all devicelab tests (see /dev/devicelab/ in this repo). You can run them locally in theory.

teriyakijack added a commit to teriyakijack/flutter that referenced this pull request Jul 17, 2018
@teriyakijack
Merge branch 'flutter_master'
7de8565 
* flutter_master: (810 commits)
  Revert engine roll to 316b026
  roll engine to 316b026 (flutter#19419)
  Revert "enable lint unnecessary_const (flutter#19342)" (flutter#19423)
  enable lint unnecessary_const (flutter#19342)
  Chevrons in month picker are semi-transparent when the month is scrolled (flutter#19363)
  Revert "Use FlutterProject to locate files  (flutter#18913)" (flutter#19409)
  Extra debug information in run_release_test (flutter#19405)
  Fix typo (flutter#19402)
  Use FlutterProject to locate files  (flutter#18913)
  Revert "roll engine to 9af920e (flutter#19365)" (flutter#19376)
  roll engine to 9af920e (flutter#19365)
  increase cache size if image is loaded that is larger than max size (flutter#19352)
  Add Bash and Zsh command-line completion for flutter (flutter#19243)
  Support keyboardAppearance field for iOS (flutter#19244)
  Add option to silence driver extension errors (flutter#19247)
  Add HeroController to CupertinoApp (flutter#19326)
  have text finder convert Text.rich to plain text for comparison (flutter#19270)
  Correct contentPadding type in InputDecoration.debugFillProperties (flutter#19318)
  Revert "Revert "Use runTests in fuchsia tester. (flutter#19178)" (flutter#19321)" (flutter#19327)
  Roll engine to c5a63d (flutter#19288)
  ...
@a14n
Copy link
Contributor Author

a14n commented Jul 17, 2018

I think the failures come from tests running in Dart1 (dev/devicelab/bin/tasks/hot_mode_dev_cycle__benchmark_dart1.dart and dev/devicelab/bin/tasks/hot_mode_dev_cycle_ios__benchmark_dart1.dart).

AFAIK it's not possible to use optional new/const with dart1 so the only solution seems to remove the tests running in dart1 (and also all the remaining dart1 things)

WDYT?

@aam
Copy link
Member

aam commented Jul 17, 2018

Good point, @a14n . @cbracken has PR that will remove dart1 tests: #19430

@jakemac53
Copy link
Contributor

jakemac53 commented Jul 17, 2018 •
edited
Loading

I tried compiling the pub package with and without new (using dartfmt --fix -w .), and there was no diff in the output of the kernel dump tool (pkg/kernel/bin/dump.dart in the sdk). There was a difference in the actual byte content but that is expected due to the source information being contained in the kernel files.

I also did not observe a meaningful difference in compile time, 0m13.391s vs 0m13.357s.

I tried a basic benchmark with millions of allocations and couldn't repro a slowdown there either.

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request Jul 19, 2018
@a14n
enable lint unnecessary_const (flutter#19342)
e5439e4
DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request Jul 19, 2018
@Hixie
Revert "enable lint unnecessary_const (flutter#19342)" (flutter#19423)
51bbe1c 
This reverts commit cc1cf13.
@a14n a14n mentioned this pull request Jul 20, 2018
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@a14n @Hixie @aam @jakemac53 @googlebot