Sdk: Add a hint: "@internal" annotation

[matanlurey]

Proposed specification:

Used to annotate compilation unit `c`.

Indicates that the unit should not be used outside of this package, and that any breaking changes that occur to the unit will not be considered a breaking change for the package.

Tools such as the analyzer, can provide feedback if:
- the compilation unit is made visible in a dart file residing in a "public" file (i.e. not in lib/src)

I'm finding this increasingly necessary when working on large packages.

For example, assume I have the following in lib/src/internal.dart:

import 'package:meta/meta.dart';

@internal
class KeyValuePair<K, V> {
  final K key;
  final V value;
  const KeyValuePair(this.key, this.value);
}

And later in my application I write the following in lib/my_package.dart:

export 'package:my_package/src/internal.dart`
// ^ WARNING: Exporting symbol `KeyValuePair`, which is marked `@internal`.

[bwilkerson]

According to https://www.dartlang.org/tools/pub/package-layout#implementation-files, everything inside the 'src' directory is already suppose to be treated as internal implementation detail. Would it be sufficient to have a lint rule that flagged uses of code inside 'src' from anywhere outside the defining package?

[matanlurey]

Not really, because you actually _would_ want to export public classes. For example:

// Assume this is lib/my_package.dart

// OK!
export 'src/internal.dart' show PublicInterfaceIWantToExpose;

// Oops!
export 'src/internal.dart' show ScaryThingIMeantToKeepPrivate;

Lots of the SDK/core packages work this way

[bwilkerson]

The lint rule I was proposing would only create a lint when a library in another package's 'src' directory is being imported. It wouldn't check for uses of re-exported classes.

[matanlurey]

The @internal is more for the package author _themselves_ to keep their public API under check.

[srawlins]

I like this annotation. In addition to the original end goal:

the compilation unit is made visible in a dart file residing in a "public" file (i.e. not in lib/src)

the analyzer should warn if an @internal identifier is being referenced outside a package. The problem now is: what is a package? As you note at #31775, it's a pub concept. I don't think analyzer has any code that identifies (A) what "package" a library comes from, or (B) whether a library is in the "public" part of a package. Womp womp. Only the linter cares if you import from src/, implementation_imports.

[srawlins]

As far as your original goal though, you don't need any concept of whether two libraries are part of the same package (A), you just need the public API concept (B). Maybe this can be simply defined:

File paths that match ^.*/lib/[^/]+.dart$ comprise the public API. Files that do not match do not.

• /Users/me/code/lib/a.dart public
• /Users/me/code/lib/src/fakeout/lib/b.dart public
• /Users/me/code/lib/src/a.dart not public
• /Users/me/code/lib/sublib/public-api/a.dart not public
• /Users/me/code/test/a.dart not public

I think this is better than looking at how libraries were imported. I'm pretty sure relative import paths could skirt any enforcement at that level. Import tracking would also require following imports and exports all the way up to an element's definition.

[lrhn]

How do you handle a library in lib exporting a library in lib/src?

I'd rather go for defining libraries being in "the same package" as having URIs starting with package:pkgname/ with the same pkgname.

This is not a language semantics issue, just an optional analyzer hint, so there is no harm in understanding pub packages - it's tooling, just like pub itself.

[srawlins]

But in what sense does a library "have a URI"? For example, if the analyzer is examining /Users/me/code/test/foo_test.dart (with a package root of /Users/me/code), what URI does it "have"? If that file has some imports:

import 'test_helper.dart';
import '../lib/src/foo.dart';

What URIs do those "have"?

[lrhn]

Inside Dart, all libraries have one exact URL, all distinct (because if they are the same, it's the same library). This is well defined, and hasn't changed in a long time. You can import the same file twice, using different URLs, and it will be considered two different libraries (introducing different classes and having separate global states), which is why you should avoid doing that.

Imports with relative paths are resolved against the library URL.
In this case, most likely the URL of the foo_test.dart file is file:///Users/me/code/test/foo_test.dart, and the URL of the second import is therefore file:///Users/me/code/lib/src/foo.dart. That's different from package:code/src/foo.dart, so you import both, you will get two different libraries .

So, import the foo.dart file as package:code/src/foo.dart. In fact, all files outside of the lib dir should use the package URI to reference files inside the lib dir, so that the actual package libraries are always referenced in the same way.

[matanlurey]

You can import the same file twice, using different URLs, and it will be considered two different libraries (introducing different classes and having separate global states), which is why you should avoid doing that.

IMO we should remove this. Made sense for Dartium, doesn't make sense anymore.

[srawlins]

@lrhn Thanks this is a good rigid definition. But this means that most packages are written so that none of their libraries are in the same "package." E.g. in the collection package, lib/src/collection.dart exports its internals via relative URIs:

export "src/algorithms.dart";
export "src/canonicalized_map.dart";

So if I have a file like:

import "package:collection/collection.dart";

then there is:

• a library with URI package:collection/collection.dart,
• a library with URI file:///Users/me/.pub-cache/hosted/pub.dartlang.org/collection-1.14.5/lib/src/algorithms.dart, and
• a library with URI file:///Users/me/.pub-cache/hosted/pub.dartlang.org/collection-1.14.5/lib/src/canonicalized_map.dart

So none of them are in the same package by your proposed definition :frowning_face:

[lrhn]

Since import and export resolution is relative to the URI of the importing library, the export "src/algorithms.dart"; in the library package:collection/collection.dart declaration exports the library package:collection/src/algorithms.dart.

That's not new, it's how it has always worked. It's also why I always use relative URIs inside the lib directory, but never in or out of it.

If you get into a situation where two files in a the same Dart package (in the lib directory of the same pub package) would not be considered to be in the same package by my definition ("look at the package name in the package URI"), then you are probably going to have problems anyway.

[srawlins]

I see, my bad. This looks good then. Probably a valid, concise definition of package that we can work with. Thanks @lrhn !

[zoechi]

Warning about library imports form other packages lib/src/... is already covered by the linter.
This indicates to me that the analyzer actually supports the concept of a package (or there is some other way to get that information, but linter rules are considered to be fast, so at least it doesn't seem to be expensive)
http://dart-lang.github.io/linter/lints/implementation_imports.html

[srawlins]

@zoechi Yeah we might be able to use the linter rule's logic about whether two URIs are from the same package, and whether a URI is private implementation. The linter's rule just asks if the first path segment is "src" though, which doesn't cover relative imports.

[nex3]

It's worth noting that if we get @internal, it would also be important to exclude @internal members from Dartdoc documentation.

[bwilkerson]

I'm not opposed to the idea, I just don't have cycles for it right now. But I will suggest that "internal" is rather general, and perhaps something more specific (like "notForExport") might be better.

Also, is it more common to want classes in src to be kept private or to be re-exportable? The annotation should go on the exception to the rule rather than on the common case, so perhaps a "forExport" annotation would be more appropriate.

[nex3]

@bwilkerson asked me to write up a proposal for the semantics of @internal I'd like to see. Here's a proposed docstring:

The annotation @internal marks an API as package-internal.

A package-internal API is intended to be used only within the package that defines it, and any breaking changes to that API will not be considered breaking changes for the package that defines it.

The @internal annotation applies to libraries, top-level declarations (variables, getters, setters, functions, classes, and typedefs), class-level declarations (variables, getters, setters, methods, operators, and constructors, whether static or not), named optional arguments and trailing optional positional parameters.

Being package-internal is transitive:

• If a library is package-internal, so is every member of it.
• If a class is package-internal, so is every member of it.
• If a variable is package-internal, so are its implicit getter and setter.

A tool that processes Dart code may report when:

• the code imports an internal library from another package.
• the code exports an internal library or member of a library from another package.
• a library outside of lib/src exports an internal library or member of a library from the same package.
• the code refers statically to an internal declaration from another package.
• the code dynamically uses a member of an object with a statically known type, where the member is internal on the static type of the object and the static type comes from another package.
• the code dynamically calls a method with an argument where the corresponding optional parameter is internal on the object's static type and the static type comes from another package.

In addition, Dartdoc should ignore any internal APIs.

[lrhn]

Consider naming it packagePrivate. The name internal is not saying what it is internal too.

Also consider making it a warning if an instance member or instance member parameter is marked internal if it overrides a non-internal member or parameter (that is, the API feature is already public in the super-class, so marking it internal will not prevent its use, and is likely an error)

[nex3]

I'd be okay with @packagePrivate, although @internal matches C#'s internal keyword.

[nex3]

Friendly ping! We're getting more and more users itching to use the Dart Sass AST, and this is still a blocking issue.

[bwilkerson]

I strongly believe that users should not be required to annotate code in src that is not intended to be public, but should instead annotate the API that _is_ intended to be public. For larger packages I expect that most of the code in src is intended to be private, and users should _not_ be required to annotate the majority of their code.

Also, I don't understand the purpose of allowing members of a class to be marked as being private when the class is public.

[natebosch]

For larger packages I expect that most of the code in src is intended to be private, and users should _not_ be required to annotate the majority of their code.

I agree that for _larger_ packages there is likely more internal than external code, for smaller packages and utility packages I think that reverses. Even given the noise in the small package case I would prefer to positively annotate the stuff that I intend to be exported than to negatively annotate the stuff that is meant to be private. My reasoning is that if I forget to mark with @public (@forExport, @external?) I'll see it reported to me or caught by tests and I've never leaked anything I don't want downstream users to depend on. If I forget to mark with @internal and publish, I'm now in the position of publishing a breaking release to correct the mistake. It's the same reason we've started preferring to use explicit show statements on our exports.

Also, I don't understand the purpose of allowing members of a class to be marked as being private when the class is public.

+1 - I find it harder to work with classes that have a different interaction within a package than outside of it, library level privacy has always been enough for me. Though the workarounds I've seen people use (like unsafe casts to FooImpl) to keep some member effectively package private are even harder.

[nex3]

I strongly believe that users should not be required to annotate code in src that is not intended to be public, but should instead annotate the API that _is_ intended to be public. For larger packages I expect that most of the code in src is intended to be private, and users should _not_ be required to annotate the majority of their code.

I strongly disagree with this. It's a very common pattern, explicitly encouraged by the pub documentation, for packages intended for user consumption to define essentially all of their code beneath src/ and export it from a single library under lib/. I strongly suspect that if we were to do a survey of packages on pub.dartlang.org, we'd find that the substantial majority of files under src/ are exported under lib/, so to follow the principle of not requiring users to annotate the majority of their code we should make package-privacy opt-in rather than opt-out.

What's more, if we were to define all code defined under src/ as package-private, that would constitute a colossal breaking change. It would render essentially all packages that currently exist on pub.dartlang.org non-functional, as well as our users' knowledge of how to construct a package.

Also, I don't understand the purpose of allowing members of a class to be marked as being private when the class is public.

Sass has a number of class hierarchies that we want to both use internally and expose for external consumption: the ASTs for Sass and CSS, and the values that can be assigned to variables and used in CSS declarations. Both of these have some members that we don't want to support externally, such as SassArgumentList.wereKeywordsAccessed and CssNode.remove(). For performance and simplicity reasons, we also want to expose the same objects externally that we do internally. Thus, we want a way to mark those internal-only members and otherwise make the class available for external consumption.

We could theoretically create separate public interfaces and private implementation classes for all of these, but that's a huge maintenance burden. Doing that for the nine value classes was and continues to be a pain; doing it for the eighty-six AST classes just so we can hide a handful of members is too much.

[matanlurey]

I think it's clear at this point we should schedule something (offline) to discuss.

I recommend someone from the analyzer team, language team, and potentially @srawlins.

Thanks!

[nex3]

As a major stakeholder for this feature, I'd like to be involved in discussions of it.

[matanlurey]

Sure, did not mean to exclude, I just mean the potential implementors need to meet up.

[natebosch]

that would constitute a colossal breaking change. It would render essentially all packages that currently exist on pub.dartlang.org non-functional, as well as our users' knowledge of how to construct a package.

How would this be breaking? My understanding is we're only talking about static analysis which would be opt-in.

I think some of the disagreement in this thread is stemming from the fact that we're talking about different requests. Could we maybe rescope this thread to the original request and file a new issue for adding new privacy modifiers including package private?

As a reminder the original request was:

export 'package:my_package/src/internal.dart`
// ^ WARNING: Exporting symbol `KeyValuePair`, which is marked `@internal`.

If we scope this to warnings for package authors then this is not breaking for any consumer of any package.

[nex3]

The original proposed specification says that the @internal annotation would

[Indicate] that the unit should not be used outside of this package, and that any breaking changes that occur to the unit will not be considered a breaking change for the package.

These are effectively the semantics I want for package-private members. I'm worried that if we consider these two requests as orthogonal, we'll end up with two annotations with (at least) confusingly similar descriptions.

Put another way, both Matan and I want to express the idea "this thing should only be used in this package" to tools. We're interested in different ways this idea could be surfaced to different users, but the idea is the same.

[matanlurey]

Re-upping. I'd like a hypothetical @internal to have the following properties:

• _Hint_ if an internal element is export-ed and visible through a library outside of lib/src/**.dart:

  // lib/src/a.dart
  import 'package:meta/meta.dart';
  @internal
  class A {}
  

  // lib/a.dart
  // HINT: Exports class A, which is marked @internal and is not meant for external consumption.
  export 'src/a.dart';
  

• _Hint_ if an internal _member_ of a class is used outside of the package it is defined:

  // package:a/a.dart
  import 'package:meta/meta.dart';
  class A {
  @internal
  void dangerousDoNotUse() {}
  }
  

  // package:b/b.dart
  import 'package:a/a.dart';
  void main() {
  // OK
  var a = new A();
  // HINT: Member dangerousDoNotUse not meant to be used outside of package 'a'.
  a.dangerousDoNotUse();
  }
  

• _Hint_ if an @internal element is unused in the contained package (DEAD_CODE).

/cc @nex3 @srawlins @davidmorgan for thoughts.

Oops, I originally missed https://github.com/dart-lang/sdk/issues/28066#issuecomment-381261744 by @nex3 - I think this covers all the cases I'm concerned about, and would greatly help in creating the API for a large framework like Angular.

[nex3]

I'd prefer a warning to a hint for external uses of an internal member--I've been in situations before where users within Google have started using private API surfaces that weren't enforced as private and I've had been on the hook to fix their code when the API changed. I'm not sure if there's latitude for the analyzer to decide something's a warning, though, and I could live with a hint.

[matanlurey]

I think semantically the team has been resistant to adding warnings or errors that are entirely metadata driven, and I think that is mostly the right tradeoff. In practice users _externally_ have been all over the place, but internally we can easily treat any specific _hint_ as fatal (we do for many, if not all, hints).

[kevmoo]

Piling on again, since I just hit this.

Here's my "vision" from my closed issue:

Used to annotate a declaration in a lib/src directory of a package that should not be exported by that package or any other and that should not be exposed (via return type, parameter type, field/property type, etc) of any other public (non-internal) member of the library.

Tools, such as the analyzer, can provide feedback if

• the annotation is associated with a declaration outside the lib directory or in the lib directory, but not under lib/src of a package
• the annotation is associated with a declaration that is exported by a library under lib, but not under lib/src
• the annotation is associated with a declaration that is exposed (via return type, parameter type, field/property type, etc) of any other public (non-internal) member of the library
• the annotation is associated with a non-public library member
• the annotation is associated with a declaration that is not used

[kevmoo]

I'm super excited about the ability for this annotation to enable cleanup of public, but unexposed members.

Right now, there is no easy way to find and delete debugDartv1SillyHelper() in lib/src/utils.dart.

[bwilkerson]

@kevmoo Some of what you specified isn't feasible because it requires global analysis. For example:

the annotation is associated with a declaration that is exported by a library under lib, but not under lib/src

Providing feedback like this would require that when we find an annotated declaration we then look at all of the public libraries to see whether any of them contain an export of the annotated declaration.

Is that what you were attempting to describe, or do you actually want the opposite (that we provide feedback is we find an export of an annotated declaration)?

[kevmoo]

@kevmoo Some of what you specified isn't feasible because it requires global analysis. For example:

the annotation is associated with a declaration that is exported by a library under lib, but not under lib/src

Providing feedback like this would require that when we find an annotated declaration we then look at all of the public libraries to see whether any of them contain an export of the annotated declaration.

Is that what you were attempting to describe, or do you actually want the opposite (that we provide feedback is we find an export of an annotated declaration)?

I feel like we're saying the same thing. imagine lib/foo.dart with export "src/impl.dart show internal; where internal is annotated. This should cause a warning/error/hint/whatever

[bwilkerson]

Ok. My confusion came from the way it was worded. The first bullet item clearly means "produce a hint at the point of declaration". The similarity of the sentence structure led me to conclude that the second bullet point meant the same thing. (Probably too much time spent reading the spec. :-)) Producing a hint at the point at which the item is exported is both feasible and sensible.

[nex3]

To reiterate, the behavior @kevmoo outlines in https://github.com/dart-lang/sdk/issues/28066#issuecomment-468403770 is not sufficient for Sass's needs. We also want the analyzer to provide feedback if a member of a type marked as @internal is referenced outside of the package that declares that type. For example:

```dart
// package:sass/sass.dart
abstract class CssNode extends AstNode {
@internal
bool get isGroupEnd;

// ...
}

// myapp/sass_processor.dart
import 'package:sass/sass.dart';

void main() {
CssNode node = /* ... */;

print(node.isGroupEnd); // Prints a warning.
}