Skip to content

@Since is not giving a warning when used from a package with a lower sdk constraint #48781

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

Closed
bsutton opened this issue Apr 10, 2022 · 14 comments
Labels
legacy-area-analyzer Use area-devexp instead. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@bsutton
Copy link

bsutton commented Apr 10, 2022

I'm not certain this is the correct spot but test is the only way I can see this working so here I am.

I managed to get myself into a right pickle and it's taken me the best part of a day to resolve.

I release a package to pub.dev with a min dartsdk of 2.12 in pubspec.yaml.

The problem was that I was running dart 2.16 on my dev/test system.
I added a feature in the code base which was from dart 2.16.
I successfully ran the unit tests and then published.

So the problem is when a user has dart 2.12 installed.

When a user tries to install my package (it's a CLI script) the install fails because pub global activate searches for the most recent package version that supports dart 2.12.
As soon as they try to run my cli script it fails because the 2.16 method I call doesn't exist and the dill compilation fails.

While the above problem is entirely my fault I really think we should have some systems in place to stop this type of mistake.

I can only think of a few possible solutions:

  1. some static analysis process as part of pub publish which checks that the API's used are part of the publish match the pubspec.yaml.
  2. pub publish warns if the local SDK version doesn't match the min pubspec version.
    This doesn't seem particularly useful.
  3. have the test package check for a matching SDK and warn/error the user if they don't match.
    Error would be my preference with a switch to ignore the problem.
@jakemac53
Copy link
Contributor

In general new APIs should have the @Since annotation, which the analyzer knows about and will warn you if you are using an API that doesn't exist in your min SDK version.

Was there a new api added that is missing this annotation?

@jakemac53 jakemac53 transferred this issue from dart-lang/test Apr 11, 2022
@jakemac53 jakemac53 added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Apr 11, 2022
@jakemac53
Copy link
Contributor

tentatively assigning this to the core libs area, but we need more info for it to be actionable. If an annotation was missed there isn't much we can do about previously published SDKs, but we could add the annotation for future sdks.

@bsutton
Copy link
Author

bsutton commented Apr 11, 2022

This is the method that caused the problem:

/// Throws [error] with associated stack trace [stackTrace].
  ///
  /// If [error] extends [Error] and has not yet been thrown,
  /// its [stackTrace] is set as well, just as if it was thrown by a `throw`.
  /// The actual stack trace captured along with the [error],
  /// or set on [error] if it is an [Error],
  /// may not be the [stackTrace] object itself,
  /// but will be a stack trace with the same content.
  @Since("2.16")
  static Never throwWithStackTrace(Object error, StackTrace stackTrace) {
    checkNotNullable(error, "error");
    checkNotNullable(stackTrace, "stackTrace");
    _throw(error, stackTrace);
  }

But as you can see it has the @Since directive.

@bsutton
Copy link
Author

bsutton commented Apr 11, 2022

I"ve attached a sample app that demonstrates the issue.

since.zip

The pubspec.yaml is set to sdk: '>=2.12.0 <3.0.0'

bin/since.dart makes a call to Error.throwWithStackTrace

My dart version is:

Dart SDK version: 2.16.2 (stable) (Tue Mar 22 13:15:13 2022 +0100) on "linux_x64"
art analyze
Analyzing since...                     0.7s
No issues found!

@bsutton
Copy link
Author

bsutton commented Apr 11, 2022

And the api documentation on throwWithStackTrace is thoroughly incomprehensible.

@jakemac53
Copy link
Contributor

I wonder if this is because the API is called under bin and not lib? There may be a bug where it isn't treating bin as a public library, but it should be.

@jakemac53
Copy link
Contributor

Huh, I can reproduce this on files under lib as well. It seems to me like @Since is possibly broken?

@jakemac53 jakemac53 added legacy-area-analyzer Use area-devexp instead. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Apr 11, 2022
@jakemac53 jakemac53 changed the title Warn users if dartsdk version on path doesn't match pubspec version @Since is not giving a warning when used from a package with a lower sdk constraint Apr 11, 2022
@bsutton
Copy link
Author

bsutton commented Apr 11, 2022 via email

@bsutton
Copy link
Author

bsutton commented Apr 11, 2022 via email

@lrhn
Copy link
Member

lrhn commented Apr 12, 2022

I'm not sure the analyzer actually checks Since annotations at all.

The original bug which made us introduce Since was #34978
It's not closed yet, but that can mean anything for such an old and forgotten bug.
(The Future/Stream exported by dart:core is probably special-cased, the error message seems far too specific.)

@bsutton
Copy link
Author

bsutton commented Apr 12, 2022

So assuming the analyser doesn't support since, it sounds like full implementation may be some way down the path.

So I would like to suggest placing some interim help in place in the way of the suggested warning.

Pub publish and test seen logical places.

If the current sdk doesn't match the pubspec min we issue at least a warning and preferably an error with a possible switch to ignore the error.

Leaving things as they are feels problematic.

@jakemac53
Copy link
Contributor

cc @bwilkerson can you clarify about @Since possibly? Is that intended to be supported? Should this change to a feature request?

@bwilkerson
Copy link
Member

Looking briefly at the code base, it does not appear that there is any support in the analyzer for the @since annotation (if there is it isn't implemented the way other checks are implemented). As far as I'm aware, the analyzer is the correct place to implement such checks, and yes, the analyzer is intended to support checking for all of the annotations in both the SDK libraries and package:meta. I don't know whether that makes this a feature request or a bug.

Adding a check is a fairly small amount of work, but I can't speak to when it will be implemented by the analyzer team. Of course, contributions are always welcome.

@jakemac53
Copy link
Contributor

Ok, I am going to go ahead and just close this issue then, in favor of #34978

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants