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

Rephrase null safety mode #74306

Merged
merged 1 commit into from Jan 20, 2021
Merged

Conversation

mit-mit
Copy link
Member

@mit-mit mit-mit commented Jan 20, 2021

The new "compiling with unsound null safety" mode output was intended for the case where an app opts into null safety, but has a language version marker in the entry point opting that file out (thus resulting in unsoundness). However, we also show it for apps that don't opt into null safety at all (e.g. existing apps still on a Dart 2.9 constraint, but compiled with a 2.12 SDK). This is an artefact of how language versioning works; the entry point in both cases uses a version below 2.12.

Changing language versioning, or making more advanced heuristics, would be complicated at this point. I suggest we rather rework the message to work for both cases: "Building without sound null safety"

cc @jonahwilliams @leafpetersen WDYT?

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 20, 2021
@google-cla google-cla bot added the cla: yes label Jan 20, 2021
@@ -67,7 +67,7 @@ abstract class BuildSubCommand extends FlutterCommand {
globals.printStatus('💪 Building with sound null safety 💪', emphasis: true);
} else {
globals.printStatus(
'Building with unsound null safety',
'Building without sound null safety',
Copy link
Contributor

Choose a reason for hiding this comment

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

While I think this is better, I'm not sure as a user what I'm supposed to do about this.

Flutter's style guide says log messages must be actionable by the user. Can this log be improved such that it alerts the user that they're mixing null-safe and non-null-safe packages, and they therefore do not get sound null safety?

If not, can we just ditch the log?

Copy link
Member Author

Choose a reason for hiding this comment

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

As a user you are supposed to be reminded that you are in an incomplete migration state, and take steps to move towards completing it. The origin of this was dart-lang/sdk#44234, which was exactly about a case like that.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

I think its worthwhile to clear up the runtime mode, at least initially when the feature is new. Its not the most actionable thing in the world, but "what mode am I running in" is difficult to answer otherwise.

@dnfield
Copy link
Contributor

dnfield commented Jan 20, 2021

Could we at least add a link to something that explains what this means and how the user can get to sound mode?

@jonahwilliams
Copy link
Member

If you expand the diff a bit, it is already there

@dnfield
Copy link
Contributor

dnfield commented Jan 20, 2021

Ahhh ok, GitHub was tricking me.

I'd still like to see this be more actionable - e.g. I think for a number of users they won't be able to migrate for a while because some dependency they don't control isn't migrated - but at least there's some explanation in there of what to do.

@mit-mit
Copy link
Member Author

mit-mit commented Jan 20, 2021

I think for a number of users they won't be able to migrate for a while because some dependency they don't control isn't migrated

The general user is not expected to start migration to null safety if that puts them in a state where they can only partially migrate. Our tooling (e.g. pub outdated --mode=null-safety and dart migrate) is setup to heavily discourage starting before all deps are done, and our documentation similarly discourages it: https://dart.dev/null-safety/migration-guide#step1-wait

But, because we've chosen to make null safety opt-in based a consequence is that developers might find themselves in these kinds of intermediate stages where they only get partial null safety. Until we make null safety mandated, we think there is value in communicating what state the developer is in.

@mit-mit mit-mit merged commit 914c9aa into flutter:master Jan 20, 2021
@mit-mit mit-mit deleted the nullsafetybuildmode branch January 20, 2021 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet

3 participants