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

[flutter_tools] Remove sound null safety flag #120936

Merged

Conversation

christopherfujino
Copy link
Member

@christopherfujino christopherfujino commented Feb 17, 2023

Fixes #118810

Remove both the --sound-null-safety and --null-assertions flags from the Flutter CLI tool. These will be removed from the Dart SDK in the 3.0 release. Before this change, we were defaulting to --sound-null-safety and --no-null-assertions, and providing a warning if the user explicitly overrode this that they were going to be deprecated soon.

Note there is no change in this PR related to the --native-null-assertions flag; it defaults to on but the user can explicitly disable it, which is the desired behavior per #118810 (comment).

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 17, 2023
@christopherfujino christopherfujino force-pushed the remove-sound-null-safety-flag branch 3 times, most recently from 44f905d to 6338250 Compare February 27, 2023 20:46
@christopherfujino christopherfujino marked this pull request as ready for review February 28, 2023 02:19
@flutter-dashboard flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Mar 6, 2023
@@ -168,7 +167,6 @@ require.config({
define("$bootstrapModule", ["$entrypoint", "dart_sdk"], function(app, dart_sdk) {
dart_sdk.dart.setStartAsyncSynchronously(true);
dart_sdk._debugger.registerDevtoolsFormatter();
dart_sdk.dart.nonNullAsserts($nullAssertions);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nshahan is it ok that I deleted this line? Or should I leave this line and hard-code the value (since I deleted the CLI arg in this PR)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, looks like good cleanup. This is totally unneeded as long as you are prevented from passing
--no-sound-null-safety when compiling. It will be defaulted to false in the compiled SDK until it is fully cleaned up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@christopherfujino
Copy link
Member Author

FYI @a-siva

@@ -91,12 +91,7 @@ enum HostArtifact {
webPlatformDart2JSSoundKernelDill,

/// The precompiled SDKs and sourcemaps for web debug builds.
webPrecompiledSdk,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this cleanup related to the flag removal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, these are the unsound parts of the web sdk, which are no longer reachable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This #122215 is tracking not building it

}
flutterUsage.sendEvent(kNullSafetyCategory, 'runtime-mode', label: nullSafetyMode.toString());
flutterUsage.sendEvent(kNullSafetyCategory, 'stats', parameters: CustomDimensions(
nullSafeMigratedLibraries: migrated,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these fields be removed from the CustomDimensions class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh, good catch

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about this diff 3519e1f

@@ -225,7 +225,7 @@ class TestCase {
for (final File test in tests) {
final String testPath = path.join(path.dirname(test.path), 'lib', path.basenameWithoutExtension(test.path));
final ProcessRunnerResult result = await runner.runProcess(
<String>[flutter, 'test', '--enable-experiment=non-nullable', '--null-assertions', testPath],
<String>[flutter, 'test', '--enable-experiment=non-nullable', testPath],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that non-nullable experiment flag used anymore? If this is the Dart SDK experiment flag it is no longer needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, good catch

@christopherfujino christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 17, 2023
@auto-submit auto-submit bot merged commit 7c3088c into flutter:master Mar 17, 2023
128 checks passed
@christopherfujino christopherfujino deleted the remove-sound-null-safety-flag branch March 17, 2023 17:48
christopherfujino added a commit that referenced this pull request Mar 17, 2023
zanderso pushed a commit that referenced this pull request Mar 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 18, 2023
christopherfujino added a commit that referenced this pull request Mar 20, 2023
auto-submit bot pushed a commit that referenced this pull request Mar 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove option to pass unsound null safety flag in flutter build/run commands
3 participants