Wish for a way to test non-public APIs

[amirh]

Flutter treats all public identifiers as part of its public API and strives to guarantee backward compatibility for every public identifier that is introduced to the framework.

As private identifiers (e.g "_foo") are only accessible from the same library they are typically not accessible from tests which are a different library.

This means that every identifier that we want to directly use in a test has to be public, and we need to commit for always keeping the framework compatible with it.

Current workaround (using library parts)

One example where this made us use a dirty workaround is the animated_icons library: We need some support of vector animations, but we did not yet have the time to properly design the API and format for it.
We wanted to implement vector animation support, as an internal implementation detail, without committing to keep supporting the current API.
But we did want to test the implementation.
What we ended up doing is implementing an animated_icons library that is made of parts, and re-define the same library using the same parts in the test file so it can access the private identifiers.

This workaround is not ideal as:

• Parts are generally discouraged by the Dart community, and are not well supported by the analyzer, and the coverage tool.
  • Due to missing analyzer features (see b/71583889) we had to disable the test in some environments.
  • Due to coverage tool issues, we are limited with what can be imported in the test (see VMScript URI does not uniquely identify a script tools#437)
  • We need to maintain a double list of all file parts (one in the real library definition and the other in the test file)
• Setting the parts structure takes some extra code and effort, and it's not always "worth it" for smaller things. Which ends up with us sometimes choosing between making something public just for testing it or testing at a less granular level (or not testing) it.

[devoncarew]

@amirh, would it help here if dartdoc didn't document symbols that were marked @visibleForTesting?

(/cc @jcollins-g)

[Hixie]

Not documenting things doesn't stop people from relying on them -- if anything, it encourages people to depend on them, because they think they're using Secret APIs and those are "cool".

[bwilkerson]

Sounds like a language issue to me. Basically "library private" and "public" are not sufficient for visibility controls.

[srawlins]

What is b/71583889?

[srawlins]

Sounds like #28066 (@internal)

Or rather, a language-enforced version of #28066. At the language level, this will take many moons to spec, discuss, implement, and release, but @internal would be a nice quick indicator, basically announcing "This element can change or be deleted at any time; don't use it." Flutter tooling might be able to piggy-back onto an analyzer warning and enforce more strongly?

I imagine @protected, @visibleForTesting and @internal could all be brought into the language proper in one go. Feels janky that we have everywhere-public and library-private visibilities, and nothing else.

[Hixie]

Many of our developers don't run the analyzer. The analyzer does not stop people from depending on APIs.

[matanlurey]

I think it would be interesting to do the following work:

1. Spec out what visibility constraints we want (independent of getting them in the language)
2. Spec out how they pertain to libraries, pub conventions, and packages
3. Get an agreement from static analysis tools to enforce the constraints we want as annotations

Once we get to this point, the flutter CLI could, for example, force that visibility is restricted before building or testing your application. We could do similar things for the webdev universe. None of this requires the language team (though they should be involved if possible).

From spending a bit of time researching this, I think we need to do the following:

1. Define what a package is, and whether we'll have the concept of "friend" packages.
2. Define what a library is, and whether we'll have the concept of "friend" libraries.
3. Probably implement the following annotations:
   A. @private
   B. @protected
   C. @internal
4. Have tooling enforce this

@Hixie:

Many of our developers don't run the analyzer. The analyzer does not stop people...

In Dart 2.0, all code will effectively run through the new CFE (common front-end), and it's not unreasonable to see if we can move the visibility restriction code to the CFE instead of only in the analyzer. Another option would be the Flutter CLI enforcing certain analysis checks.

[matanlurey]

(One of the hardest issues here is that the language has no concept of what a "package" is, it's a pub-only convention, and even there the various folders and conventions are quite loose and not well documented or enforced)

[bwilkerson]

I thought that 'library' was well defined by the language. Is there some question about what it means?

[Hixie]

I have no opinion on the above so long as the performance is good. The analyzer has historically not been fast enough to really be something we'd consider putting on the critical path (hot reload times are measured in milliseconds).

[matanlurey]

Some prior art:

• C#
• Kotlin
• Swift

[matanlurey]

@bwilkerson:

I thought that 'library' was well defined by the language.

It is, but the addition of part files, and the fact there isn't any "true" private (private to the class, for example), makes it not a direct replacement for private in most of the languages I listed above.

@Hixie:

The analyzer has historically not been fast enough

The CFE is going to have to be, so if we accept that premise it should be fast enough to enforce access control. I imagine the analyzer could be faster and incremental if on-disk caching was used.

[Hixie]

I imagine the analyzer could be faster and incremental if on-disk caching was used.

We also care about cold first-launch performance (though that's measured in seconds, not milliseconds), since that's the first experience our users will have.

But yeah, if the analyzer gets much faster, or if a subset of the analyzer can be placed in the critical path without a performance penalty, then that seems like a fine way to extend the language as far as I'm concerned.

[matanlurey]

@Hixie:

We also care about cold first-launch performance

I don't know the gritty details of the Flutter CLI, but when I use it for the first time on a new project it takes quite a while to prepare Maven/XCode/etc (lots of non-Dart dependencies). During this time couldn't the analyzer cache summaries of dependencies (similar to how Bazel/Blaze does it), given that those are unlikely to change?

It would make even "first" analysis pretty fast, and incremental even faster.

/cc @devoncarew

[Hixie]

There are many things we could do to make performance fast, and we should do many of them. :-)
I'm not trying to say "no" to the ideas above; I just want to make sure that we keep performance in mind when implementing them. I'm trying to prevent a future where a patch lands that regresses performance numbers we care about, and then we get all flustered at each other because it wasn't clear that performance matters. :-)

[zanderso]

A hack that might not be relevant for Flutter: if you know the name of a public static method of a library private class, then it can be accessed from outside the library using mirrors.

[Hixie]

One similar trick we use a lot in Flutter is to have a private class, obtain an instance of that class through various shenanigans, and then cast it to "dynamic" and call the non-underscore-prefixed members of that class directly.

[lrhn]

The usual approach is to put private things in separate libraries /lib/src/ and thereby not promise not to break it.

If you really want to access it, you can, but it's painfully obvious that you are accessing internal stuff that isn't part of the public API.

That's not a hard restriction, someone can choose to use those libraries and only get burned when they change.

I'm not opposed to making some parts of packages "package private", say anything with a path part staring with _. Then package:foo/src/_reallyInternal/foo.dart can only be imported by libraries in package:foo.

That doesn't help with testing because the test files are typically not in the library they test, they live on the side.
For testing, there is a the option of providing a command line flag to make the main entry point morally belong to a package, even if its URI is a local file URI. That is, make one library (and anything it imports with relative paths) count as belonging to one specific package. That would allow testing-only override of "package privacy" without making it a practically useful feature that you can use in large-scale programs. We don't want programs to dodge privacy just because they can.

That's an approach for "package privacy" (@internal), there is still no override for library privacy. You have to make the testable feature a public part of a private library for testing (which is still better than making it completely public and marking it as @publicForTesting).

Any analysis-based access restriction on objects can be avoided by using dynamic access, so it will not actually prevent access by a determined user. Either the method is accessible, or it's not.

I find that class-based restrictions (the normal class based "private" and "protected" from Java) doesn't work very well in Dart. I would personally prefer instance-based restrictions. A private field can only be accessed from an instance member of the object itself (that is, only through this access), not just from any member of the class. It can only be accessed through this, and you don't have the correct name to access it through dynamic.
A protected member can only be accessed from an instance member of a class extending the class declaring the member. You can only access it through this or super, and it doesn't work through dynamic either.

If we ever put something like that into the language, in a language with dynamic access, that would be the path of least resistance. Then private and protected members could be name-mangled, and only the statically detected accesses will work. Casting an object to dynamic and then trying to access a protected member will not succeed.

[devoncarew]

The analyzer has historically not been fast enough to really be something we'd consider putting on the critical path (hot reload times are measured in milliseconds).

Just for clarity, the analysis server is able to respond to deltas in tens of milliseconds (code completion, recalculated errors and warnings, ...), and hot reload is measured in hundreds of milliseconds. The analysis server is plenty fast enough for incremental changes and has been since the analysis driver work, cira Q4 2016. We do need to port flutter analyze over to being analysis server based in order to take advantage of the driver and cached analysis summaries.

[Hixie]

That's not a hard restriction, someone can choose to use those libraries and only get burned when they change.

In practice, our customers will not put up with us breaking their code, regardless of whether we're breaking them by changing a public API or a private API they happen to rely on. As a result, we consider any publicly-accessible member to be part of our public API, and thus export all of it; as a rule we have no publicly-available API that we don't export and document.

the analysis server is able to respond to deltas in tens of milliseconds

In my experience, this is a best-case scenario. I frequently see the analyzer server takes tens of seconds to process a change.

[matanlurey]

@Hixie:

In practice, our customers will not put up with us breaking their code, regardless of whether we're breaking them by changing a public API or a private API they happen to rely on. As a result, we consider any publicly-accessible member to be part of our public API, and thus export all of it; as a rule we have no publicly-available API that we don't export and document.

👍 . This comes up a lot in rolling packages in google3. Unless we have enforced unified tooling with access/visibility modifiers, a convention or "hint"-based annotation is not enough to consider this issue resolved.

[jcollins-g]

@matanlurey +1 to specing this out. While I'm sure not as badly as pub, dartdoc feels the pain of this not being specified as it has to make decisions about symbol public/private status, and encode these ad-hoc conventions.

[matanlurey]

I will add, we could always make it some sort of unofficial specification if the language team (@leafpetersen @lrhn) can't make a decision yet, but definitely having analyzer, pub, dartdoc all make their own decisions isn't the way to go.

[srawlins]

In terms of @amirh's original request here, I think the simplest solution would be friend libraries, and some of @matanlurey's other comments. So you might have:

library flutter.material.animated_icons;
friend '../../test/material/animated_icons_test.dart';

int _i = 7;

class A {
  int _j = 9;
}

• Top-level private elements are accessible within their library, and friend libraries.
• Private members of classes are accessible within their library, and friend libraries.
• This avoids introducing "package-level" visibility as new territory.
• the ../../test/... is ugly and fragile, but there is no package: URI for tests (another language shortcoming, IMO), and most packages don't have deep directories.
• I think I like the super specificity of friends here; you're not allowing access to all tests (unrelated tests) under the package, just specific ones that need the visibility.
• This would be a language feature, enforced by the runtimes.
• I think this is a very clean analog of C++'s friend concept, as private in C++ is class/member-level, not file/package level. Our friend concept can be library-level, since our _private is library-level.

[Hixie]

Seems reasonable. We could even go further and replace the "friend" keyword with "test".

[bwilkerson]

Re: friend libraries

There was a recent request to broaden @visibleForTesting to allow code in a test package to access such members without warning, and one suggestion even went so far as to ask all code in a directory named test to be able to access the code. I think we need to do some research to see what uses cases prompted these requests and whether friend (or test) would satisfy those use cases as well.

[rmacnak-google]

Duplicate of #1882

[feinstein]

How about this: the test folder structure should reflect the implementation folder structure. If both structures match, then a test file is seen as if it was an extension of the implementation file, hence they are from the same library and the test file can see the whole implementation file, as if they were the same file.