Repository navigation
Conversation
justinmc
left a comment
There was a problem hiding this comment.
I'm definitely on board with having recommendations for when to use enum vs. static const.
My main concern is that allowing non-exported code will result in a proliferation of private files, and that users will import them by path regardless of what the styleguide says. This could result in even more time spent dealing with breaking changes than before. Even though we state that we don't consider these to be breaking changes, usage by g3 customers and customer_tests can still disrupt us.
I think that's the key question for me, do we think this could create more problems than it solves?
Some other questions:
- Are these rules going to be enforced by our linter or analyzer? Like enforcing that things marked
@internalare not imported from outside the library. - Are we using any files that are not exported today? Besides feature-flagged experimental APIs like desktop multiwindow.
- Do you have anything in mind for where you want to start using internal code?
|
|
||
| * Declarations marked `@internal` are outside the public API contract; modifying or removing them is not a breaking change. | ||
|
|
||
| * For public APIs, a standard `enum` is used when consumers must handle all values exhaustively (making new values a breaking change), and a class with `static const` fields is used when consumers need a fallback (making new values non-breaking). |
There was a problem hiding this comment.
I agree with this. It's good to have an official recommendation for when to use these. This is even more important now that adding an enum will break the flutter/packages autoroller; we can't accidentally sneak in a new enum value under the radar any more.
| #### Placement rules | ||
|
|
||
| * `@internal` may be applied to top-level declarations and to `static` members or constructors of public classes inside `lib/src/`. | ||
| * In any file in `lib/src/` not exported by a barrel file, all public top-level declarations must be annotated with `@internal`. |
| ### Removing discouragement of `@visibleForTesting` | ||
|
|
||
| The style guide section "Avoid using `@visibleForTesting`" advises against `@visibleForTesting` on public declarations, preferring APIs testable through their public interfaces, though contributors still use it when necessary. Should the style guide remove its discouragement of `@visibleForTesting` or keep the current stance? |
There was a problem hiding this comment.
I can't use an API marked @internal in a test, right?
This styleguide rule is one I disagreed with when I first started on the team, but I've since gotten used to it. I want to say that I agree with the recommendation to not kid ourselves about visibleForTesting being effectively public, and so we should keep the rule as-is.
There was a problem hiding this comment.
I can't use an API marked @internal in a test, right?
Yes they serve different purposes. so I don't think we can replace visibleForTesting with internal
I want to say that I agree with the recommendation to not kid ourselves about visibleForTesting being effectively public, and so we should keep the rule as-is.
did you mean we should state that changing API that is visibleForTesting is not breaking change, and keep the discouragement as is?
| ### Adopting `@experimental` for cutting-edge development | ||
|
|
||
| New public APIs immediately fall under the breaking change policy, requiring an RFC, deprecation cycle, and migration guide to modify or remove. Should Flutter adopt [`@experimental`](https://pub.dev/documentation/meta/latest/meta/experimental-constant.html) from `package:meta` to exempt cutting-edge APIs from the breaking change policy? |
There was a problem hiding this comment.
We already have feature flags for this, I think no change is needed.
There was a problem hiding this comment.
The reason I bring this up is that should we abandon feature flag approach and just mark something as @Experiemental and treat the change to the API as non breaking change?
On the other hand, we can also just use @internal here as well.
Also, using feature flag requires a lot of works as well, is it really worth it if we can just document our rule and lint warning against using these cutting-edge API
There was a problem hiding this comment.
I'd lean towards avoiding @experimental in the Flutter SDK. While @experimental declares that we reserve the right to change an API, @experimental doesn't actually make the change any less impactful. I'd argue the Flutter SDK is so foundational that the impact of changing an @experimental API would be too high. Furthermore, @experimental APIs are listed during code completions, making them much more visible than @internal APIs.
... should we abandon feature flag approach and just mark something as @Experiemental and treat the change to the API as non breaking change?
In my mind, these serve different use cases:
- API that should only be used by the framework: Use
@internal. - Public API that reserves the right to change: Use
@experimental. (Though again, I don't think the Flutter SDK should use@experimental) - APIs that aren't ready to be used in production: Gate behind a feature flag that cannot be enabled on stable and use
@internal.
On the other hand, we can also just use @internal here as well.
Agreed 👍
| ### Handling feature flag files and the cross-layer import rule | ||
|
|
||
| `lib/src/foundation/_features.dart` defines `@internal` feature flags consumed by higher layers (for example, `lib/src/widgets/binding.dart` imports `../foundation/_features.dart`). Because `@internal` symbols cannot be exported by `lib/foundation.dart`. What should be done about feature flags? |
There was a problem hiding this comment.
I can think of a few approach.
- abandon the feature flags and just use internal or experiemental
- move this to a share library that is not part of the exported package. (will need to look into whether this is possible).
There was a problem hiding this comment.
I understand that feature flags are an exception to the "no relative imports across libraries" rule, but is this exception a problem in practice? This exception seems to be OK so far?
There was a problem hiding this comment.
yup, I think that is ok to keep it as exception, though we should make sure this doesn't encourage more exceptions
|
Thanks for writing this up. I agree with the underlying problems, but I think the document needs a structural rewrite. Comments are grouped below. 1. Split into separate RFCsI'd suggest splitting this into three RFCs:
I see the common theme, which is keeping APIs evolvable without going through the breaking change process. But the decisions can be made separately. The enum conventions are independent of the other two. The experimental RFC would build on Experimental APIs deserve their own RFC because they raise questions the other two don't: exempting public APIs from the breaking change policy; how 2.
|
I consider more private files are probably better, if we uses it when it fits. If we force everything to be public, we run into two problems
At least with "@internal" people will be warned to stay away from them.
I don't think so, for g3, they will get warning when they try to use the class, it is not any worse than if anything is forced to be public. for customer_tests, we can reject tests that use direct import. I think we will deal with breaking changes less.
I don't think so , they are currently all feature flagged based and desktop window related.
we can add lint to flutter_lint if they are not already in there.
system like Focus Management exposed a bunch of stuff like FocusAttachment |
|
Hi @dkwingsmt I only talked about visibleForTesting and experimental in open question because I think they serve a slightly different purposes and I am not too sure whether we want to adopt/change them in styleguide. just put it out in open question to see if the discussion lead us toward that direction. will revise the doc a bit based on suggestion |
| * Provides compile-time exhaustiveness checking. | ||
| * Adding a value to a public `enum` is a breaking change and requires to go through the breaking change process. | ||
|
|
||
| ##### Enum-like class with `static const` values (non-exhaustive) |
There was a problem hiding this comment.
A guidance we should bubble up: your enum-like class must have at least one non-private constructor.
If you forget this, Dart's recommended lint exhaustive_cases will enforce exhaustive checks on your enum-like class: https://dart.dev/tools/linter-rules/exhaustive_cases
| - https://github.com/chunhtai | ||
| --- | ||
|
|
||
| # RFC 710.0000: Style guide changes for package private and enums |
There was a problem hiding this comment.
nit: sealed classes are as closed / frozen as enums, what about them?
|
|
||
| #### Placement rules | ||
|
|
||
| * `@internal` may be applied to top-level declarations and to `static` members or constructors of public classes inside `lib/src/`. |
There was a problem hiding this comment.
Loic talked about keeping the rules simple / easy to digest and keeping the complicated details in lint rules. It looks pretty doable to turn these rules into an analyzer plugin lint, instead of adding to the style guide (and/or using a simpler version of this in the style guide).
| Use a standard `enum` when consumers are expected to handle every case without a `default` or `_` branch (such as `Axis`, `AxisDirection`, `VerticalDirection`, and `GrowthDirection`). | ||
|
|
||
| * Provides compile-time exhaustiveness checking. | ||
| * Adding a value to a public `enum` is a breaking change and requires to go through the breaking change process. |
There was a problem hiding this comment.
nit: is a breaking change -> can be a breaking change, as those changes are not necessarily breaking according to our breaking change policy.
There was a problem hiding this comment.
There is a big grey area when the customer_test is not broken. It is that we have to use our judgement to decide whether we "think" this will break people. People don't want to treat new enum as breakage, but when combining with exhaustive_cases it almost guaranteed someone will break.
|
|
||
| ### Removing discouragement of `@visibleForTesting` | ||
|
|
||
| The style guide section "Avoid using `@visibleForTesting`" advises against `@visibleForTesting` on public declarations, preferring APIs testable through their public interfaces, though contributors still use it when necessary. Should the style guide remove its discouragement of `@visibleForTesting` or keep the current stance? |
There was a problem hiding this comment.
The
@visibleForTestingannotation marks a public API so that developers that have not disabled theinvalid_use_of_visible_for_testing_memberanalyzer error get a warning when they use this API outside of atestdirectory.This means that the API has to be treated as being public (nothing prevents a developer from using the API even in non-test code), meaning it must be designed to be a public API, it must be documented, it must be tested, etc. At which point, there's really no reason not to just make it a public API. If anything, the use of
@visibleForTestingbecomes merely a crutch to convince ourselves that it's ok that we're making something public that we should really not have made public.So rather than rely on
@visibleForTesting, consider designing your APIs so that they are directly testable using the public API, without exposing any sensitive internals.(One exception is combining
@visibleForTestingwith@protected. The@protectedannotation marks a member as one that is intended for subclasses, so it is already a public API and considered as such. The@visibleForTestingannotation in that case merely enables the member to be called directly in tests without having to create a fake subclass and without having to add//ignorepragmas.)
The existing guide seems pretty reasonable especially with the @protected exception (the member probably should be @nonVirtual too), and it doesn't say it's strictly forbidden?
|
enum rfc has been separated out to #32 |
loic-sharma
left a comment
There was a problem hiding this comment.
This looks good to me overall!
(It looks like the RFC review process asks for a design review meeting before GitHub approval)
|
|
||
| ## Context | ||
|
|
||
| The style guide discourages package-private APIs beyond library-private (`_`) declarations and requires every file in `lib/src/<layer>/` to be exported by `lib/<layer>.dart`. When an implementation detail is needed across multiple files within a layer, contributors are left with two poor options: |
There was a problem hiding this comment.
Do we have examples where implementation details / secret states have to be exposed within a layer but not to other layers? The examples listed in the next section don't seem super convincing IMO (see the comment below).
There was a problem hiding this comment.
Also flutter/flutter#193960 seems to say another motivation for using @internal is to indicate an API is experimental.
|
Looks like people are generally ok with private files that are under feature flags or cross platform code to use I think we still need to decide whether it is ok to have package private to hide internal facing class that are not of the above two categories. Looks like in flutter/packages does this a lot, but flutter/flutter is still following the rule as best as it can. Do we want to go ahead and make lift this restriction? I vote for yes, as is in this proposal, WDYT @loic-sharma @justinmc @dkwingsmt @LongCatIsLooong |
justinmc
left a comment
There was a problem hiding this comment.
Do you have any specific reasons in mind where you want to start using @internal, or is the motivation for this just the existing violations of the styleguide?
Looks like people are generally ok with private files that are under feature flags or cross platform code to use @internal and not exported in barrel export.
What do you have in mind when you say "cross platform code" here? Or do you mean cross-layer?
Looks like in flutter/packages does this a lot
One example is NoOpPage in go_router, which is probably a good usage.
| #### Enforcement | ||
|
|
||
| * Accidental public exports of `@internal` declarations are caught by the analyzer's [`invalid_export_of_internal_element`](https://dart.dev/tools/diagnostics/invalid_export_of_internal_element) diagnostic, and external usage is caught by [`invalid_use_of_internal_member`](https://dart.dev/tools/diagnostics/invalid_use_of_internal_member) and `implementation_imports`. | ||
| * Repository-specific rules (requiring unexported `lib/src/` files to use a `_` prefix and annotate public top-level declarations with `@internal`, and enforcing instance-member placement rules) will be enforced via custom analyzer lint rules. |
There was a problem hiding this comment.
+1 to automatic enforcement via analyzer.
Overall I think the recommendation in the styleguide is useful as a guideline, but clearly we don't always follow it. I'm supportive of keeping the general guideline but being open to using @internal when it's worth it, especially if we have automated enforcement.
yes, NavigatorState.finalizeRoute and FocusTraversalPolicy. invalidateScopeData are some example that I would have use
I meant cases like conditional import for web vs io, there are a bunch for example like this one https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/widgets/_web_image_web.dart |
LongCatIsLooong
left a comment
There was a problem hiding this comment.
Is there a different motivating example for introducing @internal? I don't feel the 2 focus related examples are particularly good candidates for @internal.
|
|
||
| * In any file in `lib/src/` not exported by a barrel file, all public top-level declarations must be annotated with `@internal`, subject to the cross-platform API policy below. | ||
|
|
||
| #### Cross-platform APIs |
There was a problem hiding this comment.
cc @loic-sharma and @justinmc , I added this section let me know what you think
|
|
||
| ### Package-private APIs (`@internal`) | ||
|
|
||
| Package-private APIs remain discouraged in general, rather than banned. Unexported files and package-private declarations in `lib/src/` are permitted only in the following cases: |
There was a problem hiding this comment.
I updated the wording to discourage use of package private instead of banning.
justinmc
left a comment
There was a problem hiding this comment.
A couple questions and ideas but overall LGTM 👍 . You should try to get approval from the other commenters on this file too before merging (I think that's @LongCatIsLooong, @loic-sharma, @dkwingsmt).
| * Experimental APIs that are not yet ready for public stability guarantees. These APIs must be both annotated with `@internal` and gated behind a feature flag. | ||
| * Internal-facing APIs shared across files within the same layer (`lib/src/<layer>/`) or accessed by unit tests. Contributors should use true library-private (`_`) declarations first, and only use `@internal` package-private APIs when the file size gets out of hand or there is a good reason to separate code into multiple files for readability. | ||
|
|
||
| #### Relationship to `@visibleForTesting` and `@Deprecated` |
There was a problem hiding this comment.
Consider adding this one:
@internalAPIs must be used internally. An@internalAPI that is referenced nowhere in the codebase should be removed.
I think this helps distinguish it from @Deprecated as discussed in https://github.com/flutter/rfc/pull/30/changes#r4265258174.
If you agree, do you think it would be possible to enforce this in an automated way?
| @internal | ||
| extension RouteStateInternal on RouteState { | ||
| void finalizeRoute() => _finalizeRoute(); | ||
| } |
There was a problem hiding this comment.
I would try to avoid this kind of thing unless it's really necessary. It's probably a sign that finalizeRoute should just be public. But I guess I imagine there could be a situation where we do need it.
There was a problem hiding this comment.
yup, therefore I phrase it as discourage to use @internal as general.
For this particular case, it is for separating navigator and route files, the alternative will be merging them navigator.dart and route.dart.
There was a problem hiding this comment.
I agree with not allowing @internal on instance members of exported classes, but the @internal extension isn't a good workaround for letting other files in the package use a library-private (_) member of an exported class. A same-file @internal top-level function does the same job, since Dart privacy is library-level. Your Helper example in this thread relies on the same mechanism. Compared with the extension, it doesn't need an exception to the style guide rule against extensions, and it actually triggers invalid_use_of_internal_member for external callers. The extension doesn't, because the analyzer resolves the call to the unannotated extension member rather than to the extension.
Repro (external package importing src/ directly):
// package:lib_pkg/src/route_state.dart
import 'package:meta/meta.dart';
class RouteState {
void _finalizeRoute() {}
}
@internal
extension RouteStateInternal on RouteState {
void finalizeRoute() => _finalizeRoute();
}
extension RouteStateInternal2 on RouteState {
@internal
void finalizeRoute2() => _finalizeRoute();
}
@internal
void finalizeRoute3(RouteState s) => s._finalizeRoute();// package:app/main.dart
import 'package:lib_pkg/src/route_state.dart';
void main() {
final s = RouteState();
s.finalizeRoute(); // no diagnostic
RouteStateInternal(s).finalizeRoute(); // no diagnostic
s.finalizeRoute2(); // invalid_use_of_internal_member
finalizeRoute3(s); // invalid_use_of_internal_member
}Suggestion: recommend the top-level function as the workaround. Whether to allow @internal extensions for other cases (e.g. helpers on types we don't own) is your call, but if they are allowed, @internal must go on each extension member, not only on the extension declaration.
| 1. **Exception:** Keep existing declarations as-is and treat conditional-import implementation files as an explicit exception to the rule requiring `@internal` on public top-level declarations in unexported files. | ||
| 2. **Public facade (preferred):** Annotate declarations in the implementation files with `@internal` and expose the supported API through a public facade that delegates to the selected implementation. For example: |
There was a problem hiding this comment.
I also lean towards option 2. Defining public APIs inside _-prefixed files is counterintuitive: the _ prefix reads as package-internal, so contributors editing those files won't realize they are changing public API. I don't feel strongly, though.
| #### Barrel exports and file naming | ||
|
|
||
| * Drop the requirement that every file in `lib/src/<layer>/` be exported by `lib/<layer>.dart` (superseding [flutter/flutter#108632](https://github.com/flutter/flutter/issues/108632)). | ||
| * Any file in `lib/src/<layer>/` not exported by `lib/<layer>.dart` must have a `_` filename prefix (such as `lib/src/widgets/_accessibility_evaluations.dart`), and have all public top-level declarations annotated with `@internal`, subject to the cross-platform API policy above. |
|
|
||
| * Drop the requirement that every file in `lib/src/<layer>/` be exported by `lib/<layer>.dart` (superseding [flutter/flutter#108632](https://github.com/flutter/flutter/issues/108632)). | ||
| * Any file in `lib/src/<layer>/` not exported by `lib/<layer>.dart` must have a `_` filename prefix (such as `lib/src/widgets/_accessibility_evaluations.dart`), and have all public top-level declarations annotated with `@internal`, subject to the cross-platform API policy above. | ||
| * When an `@internal` top-level declaration shares a file with public declarations, `lib/<layer>.dart` must exclude it with `hide` (such as `export 'src/widgets/foo.dart' hide InternalBar;`). Omitting `hide` in `lib/<layer>.dart` triggers [`invalid_export_of_internal_element`](https://dart.dev/tools/diagnostics/invalid_export_of_internal_element). (`hide` is not required on intra-`src/` re-exports.) |
There was a problem hiding this comment.
Is there a valid use case for a file exposing both a public declaration and @internal, or should we force users to move the @internal stuff to a separate file?
There was a problem hiding this comment.
a helper internal class that needs to access library private of a public class.
class PublicClass {
final int _internal;
}
@internal
class Helper {
void print(PublicClasss a) {
print(a._internal);
}
}There was a problem hiding this comment.
Yeah I guess that might be annoying to split into a separate file.
| * Drop the requirement that every file in `lib/src/<layer>/` be exported by `lib/<layer>.dart` (superseding [flutter/flutter#108632](https://github.com/flutter/flutter/issues/108632)). | ||
| * Any file in `lib/src/<layer>/` not exported by `lib/<layer>.dart` must have a `_` filename prefix (such as `lib/src/widgets/_accessibility_evaluations.dart`), and have all public top-level declarations annotated with `@internal`, subject to the cross-platform API policy above. | ||
| * When an `@internal` top-level declaration shares a file with public declarations, `lib/<layer>.dart` must exclude it with `hide` (such as `export 'src/widgets/foo.dart' hide InternalBar;`). Omitting `hide` in `lib/<layer>.dart` triggers [`invalid_export_of_internal_element`](https://dart.dev/tools/diagnostics/invalid_export_of_internal_element). (`hide` is not required on intra-`src/` re-exports.) | ||
| * Cross-layer imports in `lib/src/` must continue importing top-level barrel files (`package:flutter/<layer>.dart`) rather than another layer's `lib/src/` files, with `lib/src/foundation/_features.dart` as the only permitted exception for framework-wide feature flags. |
There was a problem hiding this comment.
Another thing to consider enforcing automatically if we can
dkwingsmt
left a comment
There was a problem hiding this comment.
I'm generally ok with the current direction, although I found a few non-trivial issues.
| #### Placement rules | ||
|
|
||
| * `@internal` may only be applied to top-level declarations (classes, mixins, enums, extensions, typedefs, functions, and variables) inside `lib/src/`. | ||
| * Do not apply `@internal` directly to members or constructors of a publicly exported class, because `export ... hide` cannot hide class members. Instead, keep the implementation library-private (`_`) and expose it within the layer via an `@internal` `extension` declared in the same file and excluded from `lib/<layer>.dart` with `hide` (an explicit exception to the style guide rule against `extension`s). |
There was a problem hiding this comment.
because
export ... hidecannot hide class members
This rationale doesn't hold. Exporting a public class that contains an @internal member does not trigger invalid_export_of_internal_element, and external callers who try to call that member through the barrel still get invalid_use_of_internal_member without needing hide.
The real problem is that instance members are part of the class interface, so changing an @internal instance member can still break external subclasses and implementers:
// package:flutter/src/widgets/foo.dart (exported by widgets.dart)
class Foo {
@internal
void bar() {}
}
// External package importing package:flutter/widgets.dart:
// 1. Breaks `implements`: external implementers must implement `bar()`,
// so adding, removing, or changing `bar()` is a breaking change.
class MyFoo1 implements Foo {
@override
void bar() {} // Required by the compiler; no @internal warning!
}
// 2. Breaks `extends`: an external subclass can override `bar()` with no
// warning, or fail to compile if it already has a `bar` with a different
// signature.
class MyFoo2 extends Foo {
void bar(int x) {} // Compile error: invalid_override!
}The earlier base/final/sealed discussion captured this interface problem; I'd use that as the rationale here.
There was a problem hiding this comment.
that is true, but using internal extension is safer in this case
so the code will be
class Foo {
}
@internal
class FooInternal {
void bar() {}
}
this way, unless people go ahead and force import this extension, they won't have access to bar externally, even for override.
There was a problem hiding this comment.
To clarify, I'm not saying that the point doesn't stand. I agree that @internal shouldn't be applied to methods of public classes. I'm suggesting that the RFC should phrase it with a more convincing rationale.
| @internal | ||
| extension RouteStateInternal on RouteState { | ||
| void finalizeRoute() => _finalizeRoute(); | ||
| } |
There was a problem hiding this comment.
I agree with not allowing @internal on instance members of exported classes, but the @internal extension isn't a good workaround for letting other files in the package use a library-private (_) member of an exported class. A same-file @internal top-level function does the same job, since Dart privacy is library-level. Your Helper example in this thread relies on the same mechanism. Compared with the extension, it doesn't need an exception to the style guide rule against extensions, and it actually triggers invalid_use_of_internal_member for external callers. The extension doesn't, because the analyzer resolves the call to the unannotated extension member rather than to the extension.
Repro (external package importing src/ directly):
// package:lib_pkg/src/route_state.dart
import 'package:meta/meta.dart';
class RouteState {
void _finalizeRoute() {}
}
@internal
extension RouteStateInternal on RouteState {
void finalizeRoute() => _finalizeRoute();
}
extension RouteStateInternal2 on RouteState {
@internal
void finalizeRoute2() => _finalizeRoute();
}
@internal
void finalizeRoute3(RouteState s) => s._finalizeRoute();// package:app/main.dart
import 'package:lib_pkg/src/route_state.dart';
void main() {
final s = RouteState();
s.finalizeRoute(); // no diagnostic
RouteStateInternal(s).finalizeRoute(); // no diagnostic
s.finalizeRoute2(); // invalid_use_of_internal_member
finalizeRoute3(s); // invalid_use_of_internal_member
}Suggestion: recommend the top-level function as the workaround. Whether to allow @internal extensions for other cases (e.g. helpers on types we don't own) is your call, but if they are allowed, @internal must go on each extension member, not only on the extension declaration.
|
|
||
| #### Placement rules | ||
|
|
||
| * `@internal` may only be applied to top-level declarations (classes, mixins, enums, extensions, typedefs, functions, and variables) inside `lib/src/`. |
There was a problem hiding this comment.
I'd suggest allowing @internal on constructors and static members of exported classes. Unlike instance members, they aren't part of the inherited interface—they can't be overridden or required by implements—and the analyzer enforces @internal on them (I verified named constructors, super calls, and static methods). An earlier revision of this RFC allowed this, and it seems to have been dropped without discussion when the instance-member rule was removed.
Allowing @internal on constructors also covers two cases that top-level functions and class modifiers can't handle across files:
- In-package subclassing across files. Suppose we have a public, documented base class
Fooand want subclasses in other files of the same layer to extend it, while preventing external packages from extending or instantiating it. Dart's class modifiers can't express package-scoped subclassing (finalandsealedblock subclassing even from other files in the same package, whilebasestill allowsextendsoutside the package), and a private constructorFoo._()can't be called from another file. A public class with an@internalgenerative constructor (@internal Foo.internal()) solves this: in-package subclasses callsuper.internal(), while external subclasses and callers getinvalid_use_of_internal_member. - Const instances in other files. An
@internalconst constructor (@internal const Foo.internal()) can be used inconstexpressions in other files of the layer, whereas a top-level helper function wrappingFoo._()cannot produce a compile-time constant.
Static members have no unique capability over same-file top-level functions, but there's no reason to disallow them either, and they keep class-specific helpers namespaced under the class.
There was a problem hiding this comment.
yeah inheritance is something internal extension can't solve, and a package private constructor can't be solved by extension.
maybe we should just still allow them on the instance member but prefer internal extension when possible. WDYT?
|
|
||
| #### Enforcement | ||
|
|
||
| * Accidental public exports of `@internal` declarations are caught by the analyzer's [`invalid_export_of_internal_element`](https://dart.dev/tools/diagnostics/invalid_export_of_internal_element) diagnostic, and external usage is caught by [`invalid_use_of_internal_member`](https://dart.dev/tools/diagnostics/invalid_use_of_internal_member) and `implementation_imports`. |
There was a problem hiding this comment.
The analyzer does not generally catch @internal types that leak into the public API through exported declarations. invalid_export_of_internal_element only checks declarations exported directly by name, invalid_export_of_internal_element_indirectly only covers a narrow case (an @internal typedef in the signature of an exported top-level function), and library_private_types_in_public_api only checks _-prefixed types. As a result, dart analyze reports nothing on either the framework or the consumer when an @internal type reaches users through a public member signature or supertype:
// lib/src/widgets/foo.dart, exported by lib/widgets.dart with `hide InternalState, InternalBase`
@internal
class InternalState {
void reset() {}
}
@internal
class InternalBase {
void helper() {}
}
class PublicController {
InternalState get state => InternalState();
}
class PublicWidget extends InternalBase {}// External package, importing only package:flutter/widgets.dart
controller.state.reset(); // no diagnostic
final s = controller.state; // no diagnostic; type inferred
PublicWidget().helper(); // no diagnosticLeaving this gap open creates three problems:
- External callers get no warning, yet have no stability guarantee. Consumers here neither import
src/nor suppress any warning, and automated checks onflutter/testswon't flag their code either. Yet under this RFC,InternalStateandInternalBaseare unexported@internaldeclarations, so renamingreset()orhelper()can break valid caller code without a deprecation period. - API documentation is either broken or misleading. Because
InternalStateis not exported,dart docgenerates no page for it (and emits no warning), leavingPublicController.statewith an unlinked return type and no documentation forreset(). Conversely, forPublicWidget,dart doclistshelper()onPublicWidget's page as an ordinary inherited method, giving users every reason to treat it as supported public API. - It is easy to introduce by accident. A contributor might mark an existing class
@internalwhile a public signature still refers to it, or extract an@internalbase class to share implementation across files. Because the Enforcement section implies that the analyzer already catches accidental leaks, reviewers won't know to look for this manually.
I'd suggest adding a rule, enforced by the custom lint: @internal types must not appear in the public signatures or supertypes (extends, implements, with, on) of publicly exported declarations. Instead, either make the type public and keep construction internal with an @internal constructor (see above), or expose it through a public supertype.
There was a problem hiding this comment.
yes agree. We should definitely have this rule and ci enforcing it
| 1. **Exception:** Keep existing declarations as-is and treat conditional-import implementation files as an explicit exception to the rule requiring `@internal` on public top-level declarations in unexported files. | ||
| 2. **Public facade (preferred):** Annotate declarations in the implementation files with `@internal` and expose the supported API through a public facade that delegates to the selected implementation. For example: |
There was a problem hiding this comment.
I also lean towards option 2. Defining public APIs inside _-prefixed files is counterintuitive: the _ prefix reads as package-internal, so contributors editing those files won't realize they are changing public API. I don't feel strongly, though.
|
|
||
| * `@internal` is for package-private declarations omitted or hidden from public barrel exports. | ||
| * `@internal` APIs must be used internally. An `@internal` API that is referenced nowhere in the codebase should be removed. | ||
| * `@internal` must not be used on APIs meant to be removed; use `@Deprecated` instead. |
There was a problem hiding this comment.
This rule relies on author intent, which can't be verified in review. As raised earlier in this thread, an exported API can be marked @internal (and hidden from the barrel) in one PR, and then deleted in a follow-up PR with no deprecation period. Even when the author genuinely intends to keep the API for internal use, hiding an existing symbol from the barrel breaks external callers immediately without the migration window that @Deprecated provides.
I'd suggest a mechanical rule instead: hiding an already-exported API from the barrel or marking it @internal counts as removing a public API and must go through the standard deprecation process first (deprecate the public API while migrating internal callers to a new private or @internal helper, then remove the deprecated public API after the deprecation period).
There was a problem hiding this comment.
I'd suggest a mechanical rule instead: hiding an already-exported API from the barrel or marking it @internal counts as removing a public API
It should be treated as breaking change for sure. The loop hole is that once that is done we don't have guideline says when it can be actually removed. so a bad actor can come in mark it internal (with breaking change notice) and remove it immediately after.
The deprecation policy does say we will wait at least a stable release before removing @deprecated API.
We could included that removing @internal API should also wait at least one stable, but then it still require reviewer to catch the bad actor. I felt saying more other than the current statement won't help much.
There was a problem hiding this comment.
That's a good point. Thanks for clarifying. Maybe we should rule that adding @internal to an existing API must be accompanied by a comment of the earliest date that this internal API might be broken, which is 1 year from the PR's landing date.
| #### Relationship to `@visibleForTesting` and `@Deprecated` | ||
|
|
||
| * `@internal` is for package-private declarations omitted or hidden from public barrel exports. | ||
| * `@internal` APIs must be used internally. An `@internal` API that is referenced nowhere in the codebase should be removed. |
There was a problem hiding this comment.
For the unused-@internal check, references from tests shouldn't count as uses. Test-only access is what @visibleForTesting is for. Consider clarifying whether the "nowhere in this codebase" includes unit tests.
This suggestion also means dropping "or accessed by unit tests" from the permitted cases.
|
|
||
| Package-private APIs remain discouraged in general, rather than banned. Unexported files and package-private declarations in `lib/src/` are permitted only in the following cases: | ||
|
|
||
| * Experimental APIs that are not yet ready for public stability guarantees. These APIs must be both annotated with `@internal` and gated behind a feature flag. |
There was a problem hiding this comment.
I again suggest keeping experimental APIs out of this RFC, as I mentioned earlier. How experimental APIs should work is more than just a tag, and hasn't been settled in this thread: @internal + feature flag vs. @experimental, how users opt in, how APIs graduate, and whether SDK packages may use them (flutter_test/lib/src/binding.dart already uses the windowing APIs via a file-wide ignore_for_file). I'd suggest removing the experimental category here, and treating the existing windowing APIs and the _features.dart exception as out of scope until a separate RFC covers them.
There was a problem hiding this comment.
@experimental is a weaker guard than @internal + feature flag. We also already have leaned toward package private + feature flag in our current code, this guideline change only makes it easier to follow for new experimental API. I view it as a net benefit.
There was a problem hiding this comment.
I agree that @internal + feature flag is a stronger guard than @experimental; I'm not arguing for @experimental. My concern is codifying the pattern before its rules are worked out.
The only existing case, windowing, doesn't follow the other rules in this RFC: WidgetsBinding.windowingOwner is an @internal instance member on an exported mixin, and flutter_test uses these APIs through a file-wide ignore_for_file. Once this lands, those become violations to fix or exceptions to carve out. Open questions like how users opt in (importing src/ plus an ignore?) and how an API graduates would get answered by whatever the first few cases happen to do, and changing a published guideline later is harder than getting it right once.
Since windowing is the only experimental API using this pattern today, deferring the category to a separate RFC costs little. If you'd rather keep it here, I'd at least like the RFC to cover how windowing conforms, how users are expected to opt in, and what graduation looks like.
Proposing change to style guide around package private in public APIs
Tracking issue: flutter/flutter#193960
This proposal directly contradicts flutter/flutter#108632
Pre-launch Checklist
design doc.dart run bin/assign_rfc_number.dartlocally when instructed).If you need help, consider asking for advice on the #hackers channel on Discord.