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

Warn when publishing a null-safety compatible package with non-null-safe dependencies #2438

Closed
jonasfj opened this issue Apr 7, 2020 · 12 comments

Comments

@jonasfj
Copy link
Member

jonasfj commented Apr 7, 2020

When a package is published we should warn if:

  • The package has opted into null-safety (sdk constraint lower bound of 2.10), AND,
  • A library in the package imports a non-null-safety compliant library.

Note. The second condition can be simplified into: if the package has dependency on a non-null-safety complaint package. Tracing import-paths might be preferred, because it creates fewer false positive warnings -- but it might also be too complicated.

We need to make this warning because it is highly likely that when the dependency is published in a non-null-safety compliant version the interface will be broken (whether intentionally or not).

@mit-mit
Copy link
Member

mit-mit commented Apr 7, 2020

Shouldn't this be an error?

@sigurdm
Copy link
Contributor

sigurdm commented Jun 12, 2020

Shouldn't this be an error?

I believe we want to support mixed-mode packages? @jonasfj ?

@jakemac53
Copy link
Contributor

Mixed-mode has to be supported for package cycles to be able to publish. The vast majority of package cycles are introduced due to dev_dependencies though so its possible if this is only taking dependencies into account that we could avoid the problem.

Allowing publishing of mixed mode packages in general is very problematic. We want to discourage it as much as possible without completely blocking any valid use cases that require it.

@jakemac53
Copy link
Contributor

jakemac53 commented Jun 15, 2020

I would like to propose that we add a new class of "warning" to pub, which is one that requires an explicit flag in order to bypass it.

This decreases the chance that people publish these packages by ensuring they actually read the warning message (in order to know what flag to pass to bypass it).

We should include with this warning some text along the lines of:

Note that by publishing with non-migrated dependencies your package may be broken
at any time if one of your dependencies migrates without a breaking change release.
We highly recommend that you wait until all of your dependencies have been migrated
before publishing.

To bypass this warning and publish anyways, pass
`--unsafe-publish-with-unmigrated-dependencies`

@natebosch
Copy link
Member

I'd extend that warning to include something about how if you didn't run your tests in sound null safety mode you are increasing the risk of shipping bugs.

@jonasfj
Copy link
Member Author

jonasfj commented Jun 16, 2020

Note that by publishing with non-migrated dependencies your package may be broken
at any time if one of your dependencies migrates without a breaking change release.

While it's likely that a dependency migrating will break you, it's not necessary a given, is it?


To bypass this warning and publish anyways, pass
`--unsafe-publish-with-unmigrated-dependencies`

This is certainly an interesting idea, but I think it's an orthogonal concern. Filed #2535

@natebosch
Copy link
Member

While it's likely that a dependency migrating will break you, it's not necessary a given, is it?

If you publish a migrated package with unmigrated dependencies there are a few bad things that might happen:

  • Your dependency migrates and publishes as a breaking change version bump (what they should do, and what we will advise them to do). Anyone downstream of you still can't migrate safely they can't run in strong mode.
  • Your dependency migrates and publishes as a non-breaking change. Now your package might happen to work, but if it doesn't (which is likely) all your downstream consumers are broken at those package versions with no way to get out of the problem other than pinning versions. They still can't migrate safely.

@jakemac53
Copy link
Contributor

jakemac53 commented Jun 17, 2020

While it's likely that a dependency migrating will break you, it's not necessary a given, is it?

I would argue that it is extremely likely you will be broken. There are any number of potential ways in which this can happen.

  • Any legacy api which is migrated to a nullable return type is very likely to break you
    • you could freely pass the result of that to things requiring a non-nullable parameter before, but now it will be an error
    • type inference may change so a local variable is now nullable, requiring null checks that were not required before
  • Any tearoffs of legacy functions will have much different types as well, for instance no longer accepting nullable parameters or changing the return type to be nullable which may not match the expected function types where the tearoff is used.
  • Legacy parameters accept null and non-null arguments, most of these will change to non-nullable parameters, breaking call sites to those functions which have nullable arguments.
  • And plenty more :)

This is why all NNBD releases really need to be breaking change releases.

@mit-mit
Copy link
Member

mit-mit commented Jun 24, 2020

This is why all NNBD releases really need to be breaking change releases.

I think there is an or missing here: or we put enough guardrails in place that we feel confident that either everybody migrates in order (i.e. soundly) or they migrate unsoundly knowing they are likely to get broken.

@natebosch
Copy link
Member

natebosch commented Jun 24, 2020

or they migrate unsoundly knowing they are likely to get broken.

It's not only the authors who will be broken. They can break anyone downstream of them.

I also think it's likely that authors will accidentally ship breaking changes. The breaking version bump is safer for non-migrated downstream dependencies too.

@mit-mit
Copy link
Member

mit-mit commented Sep 2, 2020

@jonasfj is this done?

@sigurdm
Copy link
Contributor

sigurdm commented Sep 7, 2020

Yes!

@sigurdm sigurdm closed this as completed Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants