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

Can the @Since annotation be made usable for packages? #52627

Open
rrousselGit opened this issue Jun 6, 2023 · 13 comments
Open

Can the @Since annotation be made usable for packages? #52627

rrousselGit opened this issue Jun 6, 2023 · 13 comments
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@rrousselGit
Copy link

The SDK uses a custom annotation for warnings: @Since('version').

This warning is helpful to help folks with changing their version constraints to match what they use in their application.
This is especially useful in Dart as it is installed globally instead of on a per-project basis. But there are other use-cases.

I think opening this annotation to Flutter and package authors would be helpful too.

In general, it is very difficult to know what is the actual minimum version possible of a dependency based on what a package uses.
A package may specify dependency: ^1.0.0, yet use a feature only available since v1.2.0 – because when the package was first developed, 1.2.0 was available.

As such, the package author may think their package is compatible with dependency: 1.1.0, when it actually isn't. So if an application depends on our package but also uses an older dependency version, it will get a funky compilation error (in the best-case scenario) instead of pub failing to resolve the pub get.

By opening up this annotation to package authors, folks could start annotating new APIs with @Since when they are introduced. This would help the community as a whole deal with minimum version constraints.

@stevemessick stevemessick added the area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). label Jun 6, 2023
@lrhn
Copy link
Member

lrhn commented Jun 6, 2023

We can make the Since class from the platform libraries public, likely in dart:core, or we can introduce a new declaration in package:meta.

The latter requires an extra dependency to use. The former exposes an annotation that has no language effect, and it's only used by the analyzer. But that goes for all other public annotations in dart:core too. Which is really just deprecated, the override should have been in package:meta too.

The Since annotation is about versioning, just like deprecated, so it does fit the theme.

@rrousselGit
Copy link
Author

Note that it appears to be quite common for package authors to forget to bump their minimum constraints.

Since I've started running pub downgrade in my CIs to check that I didn't forget to bump my min constraints, I've found that numerous dependencies also forgot to do so too.
For example, today I found that mockito forgot to bump their minimum analyzer version. dart-lang/mockito#656

This forces me to fix versions of transitive dependencies on my own in my packages, by listing them as dev_dependencies with the necessary version.

There's an example in one of the repositories I'm working on: https://github.com/invertase/dart_globe/blob/main/packages/globe_cli/pubspec.yaml#L30

@lrhn lrhn added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. and removed area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). labels Jun 17, 2023
@lrhn
Copy link
Member

lrhn commented Jun 17, 2023

Changing to be an analyzer issue, because the question is whether the analyzer is interested in checking Since annotations in package code. If so, making the annotation public would be fine.

The behavior would be similar to the SDK:

  • A declaration would be annotated with @Since("X.Y"), which should be no greater than the current version of the current package, as written in pubspec.yaml.
  • A reference to/use of that declaration from another package would cause a warning, unless the minimum version constraint on the providing package is at or above "X.Y.0".
  • If the package doesn't have a minimum constraint on the package of the declaration it's using, meaning it's not depending directly on that package, it's probably because the declaration is exported by another package. We'd have to figure out which minimum constraint to use in that case. (I'm guessing a simple heuristic will be enough most of the time, say "minimum required version by any directly depended on package which directly depends on the declaring package, and if that fails, continue a breadth first search on direct dependencies." It must come from somewhere, and likely just from one other package.)

It's not perfect for indirect dependencies, because it still depends on the current version solution. Optimally, it should depend on the result of pub downgrade. Hopefully indirect dependencies are rare enough that it's not a big issue.

@rrousselGit
Copy link
Author

About the package exporting another package issue:
Maybe export statements could have a "since" annotation?

Like:

// package:foo/foo.dart
export 'package:bar/bar.dart'
  show
    @Since('1.0.0')
    Bar,
    @Since('1.1.0')
    barUtil;

This should enable packages to "override" the @since for their use-case.

@rrousselGit
Copy link
Author

Ideally, we would also have a CI tool which, by relying on either pub or git, checks for missing @since statements.

Like maybe a custom pub publish rule.

@lrhn
Copy link
Member

lrhn commented Jun 19, 2023

About the package exporting another package issue:
Maybe export statements could have a "since" annotation?

I'd actually want that syntax for saying when the export was added, in the current package's versioning.
(Probably can't put annotations on individual show clauses, but you can have multiple exports of the same library.)

That still leaves us without a way to say which version of the exported library we're promising.
Maybe a modifier to the annotation:

@Since("1.2") // The version of my own package where this export was added.
@Since(package: "foo", "3.3"),  // The expected library version of the exported package.
export  "package:foo/foo.dart";

(Very loose thought, not sure whether it'd work.)

@rrousselGit
Copy link
Author

I think show-based @Since is quite valuable.

Because a more legitimate use case may be one package having multiple public libraries, both exporting the same private library, but with different shows/hides.

Like:

// my_package/lib/a.dart
export 'src/file.dart' 
  show
   // This library exports Foo since 1.0.0
    @Since('1.0.0')
    Foo;

// my_package/lib/b.dart
export 'src/file.dart'
   show
      Bar,
      // "Foo" is exported by this library only since v1.2.3
     @Since('1.2.3')
      Foo;

In this case, although the class Foo existed since v1.0.0, the import package:my_package/b.dart only exports Foo since v1.2.3

@keertip
Copy link
Contributor

keertip commented Jun 21, 2023

cc @bwilkerson

@keertip keertip added the P3 A lower priority bug or feature request label Jun 21, 2023
@bwilkerson
Copy link
Member

... the question is whether the analyzer is interested in checking Since annotations in package code.

If we can fully define the semantics of the annotation when used outside the SDK (there are some interesting ideas above, we just need to know which are actually being supported) I wouldn't have any problem having the analyzer check for it. The analyzer already produces warnings related to the other annotations exposed from the core libraries.

I think show-based @Since is quite valuable.

The language grammar allows annotations on directives, but doesn't allow annotations on the names in a show clause. In order to change the parser to recognize these annotations we'd probably need a minor adjustment to the language. I doubt that it would create any issues, but it can't really be an analyzer-only enhancement; I think it really does need to be added to the spec for all tools.

@lrhn
Copy link
Member

lrhn commented Jun 22, 2023

The grammar not allowing annotations inside the declaration was the main reason I suggested having multiple exports if you need multiple versions mentioned.

To actually work, each such annotated exported declaration should probably only be exported by one export.

@sigurdm
Copy link
Contributor

sigurdm commented Jun 29, 2023

cc @jonasfj who has been thinking about this.

@jonasfj
Copy link
Member

jonasfj commented Jul 4, 2023

I can see why we'd want to make the @Since annotation a thing in package:meta.
But I doubt that many package authors would actually write @Since annotations on everything.

We could score packages with @Since annotations on pub.dev, and we could make tooling to generate @Since annotations. But I still think most package authors wouldn't bother. And scoring would often be unable to detect missing @Since annotations (or it would have too many false positives).


Instead, I've been leaning towards the idea what this should be solved through static analysis. It would most definitely end up being a gross under-approximation, that can only highlight some issues, not all issues.

In the extreme scenario, we could simply grep the lower-bound version of the dependency you're using to see if the identifier you're using exists inside the package.

Example of automatic @Since warnings without annotations

Imagine that I have a project as follows:

pubspec.yaml
  dependencies:
    foo: ^1.0.0                      # A dependency on foo >= 1.0.0 <2.0.0
  ...

pubspec.lock
  foo:
    version: 1.5.0                   # But I'm actually using foo 1.5.0
  ...

bin/myapp.dart
  import 'package:foo/foo.dart';
  void main() => callingFoo();       # Line where I'm calling a method from package:foo

In the most gross under-approximation the analyzer could produce a warning at callingFoo(), if when scanning through the unresolved AST of package foo version 1.0.0 an identifier callingFoo is not declared anywhere.

Note: The under-approximation suggested "an identifier", it didn't say whether it was a class, enum, top-level variable or function. Just that the identifier doesn't exist.

We know that the invocation callingFoo() in bin/myapp.dart is likely a problem, if:

  • In the current resolution of dependencies callingFoo() is defined in package:foo, and,
  • Package foo version 1.0.0 doesn't contain an identifier called callingFoo.
  • No library in foo re-exports all identifiers from a library in another package (without using show)

Maybe there are more conditions I'm missing. This was just an illustrative example,

Certainly, this kind of under-approximations could also be refined a lot. Like we could care if it's a method / class / function / variable, etc. More research and experimentation is necessary. I think it could be fun to try and do this kind of analysis as an experiment, outside the Dart SDK to prove it viable.

I actually suspect (not sure), that it might be possible to know all exported classes, functions, enums, variables and typedefs, and their members; without resolving types. Just by scanning the AST. Maybe, with limitations when extending a class from another package, or re-exporting symbols from another package.

Probably, we could scan all package on pub.dev and offer a summary file that can be downloaded. Or it might be reasonable to have users download it, and then generate this summary in the pub client, as this would work better for people with custom package repositories.


My point is: I think an under-approximation is worth considering instead of having users write @Since annotations. Because most cases can probably be caught by a reasonably refined under-approximation (probably even if it's entirely based on an unresolved AST scan).

@rrousselGit
Copy link
Author

I think it's a case of "Why not both?"

Some package authors would be willing to use @Since (especially Flutter, popular packages, and core packages like analyzer). A static analysis could be used to help with packages which do not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

8 participants