Sdk: Two internal types created from one type declaration when a test uses relative imports

[escamoteur]

• Dart SDK Version (dart --version)
  Dart VM version: 2.9.0-7.0.dev.flutter-617bc54b71 (be) (Thu May 7 23:23:09 2020 +0000) on "windows_x64"

• Whether you are using Windows, MacOSX, or Linux (if applicable)
  Windows

A user of my package get_it raised an issue that a type that he registered in GetIt was not available when he tried to access it later.

GetIt uses internally a `map to store registered Objects. When debugging the test I observed that the type in question was indeed stored in the map and that the type that was passed to access it was the same type but still the map returned always null.
When comparing the hashcodes of both type instances they were different.

Following a hunch I changed all relative imports to package imports because there were problems before and suddenly it worked.

I tried to reproduce it in a new project but didn't succeed. So I stripped down the project that had the problem.

Please watch this screencast:
https://www.screencast.com/t/dqufsChuC

you find the project here: https://github.com/escamoteur/spacex_flights_get_it_assertion

[mit-mit]

cc @johnniwinther does this sound like a potential CFE issue?

[mraleph]

This is working as intended. There are few important parts to understand here:

• In Dart libraries are distinguished by their import URIs

• Import URIs are not necessarily the same as resolved URIs e.g. package:a/b.dart might be resolved via .package file to file:///home/mraleph/sources/a/lib/b.dart, but if I do

  import 'package:a/b.dart' as lib1;
  import 'file:///home/mraleph/sources/a/lib/b.dart' as lib2;
  

  I get two different libraries because they were imported through two different import uris.

• When you use relative imports in a library then those are resolved relative to the import URI of the library in which they appear.
• Tests are loaded using file: rather than package: URI - because tests (just like entry points) live outside of lib.

Combining this all together explains the behaviour - when test is importing other libraries using relative URIs those libraries and all other libraries they import using relative imports are imported through file: URIs because the test itself is loaded through a file: URI. Now there are also libraries in the code which are loaded through package: URI - those libraries and all other libraries they import using relative paths are imported through package: URI. This is how you end up with duplicated libraries - even though these libraries are imported from the same files they are imported using different import URIs so they end up being distinct from the language perspective and their internal state (e.g. static variables) are also distinct.

Effective Dart gives very clear guidance here, see this section:

But you can’t “cross the streams”. A library outside of lib should never use a relative import to reach a library under lib, or vice versa. Doing so will break Dart’s ability to correctly tell if two library URIs refer to the same library. Follow these two rules:

• An import path should never contain /lib/.
• A library under lib should never use ../ to escape the lib directory.

[escamoteur]

It might be as it's currently implemented in the Language, but does it make sense or does it more lead to confusion?

Also I and others don't understand wha Effective Dart make the recommendation

PREFER relative paths when importing libraries within your own package’s lib directory.

at all. It too often leads to problems. Why not always use the package import as the golden rule?

[mraleph]

I do agree that this is somewhat confusing.

/cc @lrhn to provide language design perspective on this issue.
/cc @munificent to provide Effective Dart perspective on this issue.

[mit-mit]

@escamoteur can you clarify why you prefer using a combination of import scheme for the content under /lib? I poked around in the repo you shared, and spacex_flights_get_it_assertion/lib/src/models/rocket.dart has:

import 'package:spacex_flights/src/models/second_stage.dart';

which could have been:

import 'second_stage.dart';

[escamoteur]

@mit-mit I want exactly the opposite, no combination but only package imports because if people mix imports like in the repo that I got from a user of GetIt it often leads to problems. I guess this is the same reason for the other "bug" I found yesterday
https://github.com/dart-lang/sdk/issues/41865

[mit-mit]

I believe there are three options here. Are you sure you really prefer option 2. with the very verbose imports?

1: allow mixed imports

We'd change the language and implementation to allow both style imports. I'm not an expert here, but I'd assume this is very hard, if even possible.

2: Recommend package imports

We'd change the guidance to use package imports only. That would lead to much more verbose imports, e.g. from your sample repo, main.dart would change from:

import 'package:flutter/material.dart';

import 'service_locator.dart';
import 'src/app.dart';

to:

import 'package:flutter/material.dart';

import 'package:spacex_flights/service_locator.dart';
import 'package:spacex_flights/src/app.dart';

3: Ensure projects use lints to avoid mixed mode imports

We keep the current guidance, but ensure that all project templates configure the lint to prevent this. We'd even change the severity of it to a warning, or error:

analysis_options.yaml:

linter:
  rules:
    - avoid_relative_lib_imports

analyzer:
  errors:
    avoid_relative_lib_imports: error

[lrhn]

See also https://github.com/dart-lang/language/issues/939 for a suggestion on how to reduce the problem.

My usual recommendation is that you must never have any path with lib/ in it. (Or more precisely: Never have a relative path which goes into or out of a lib/ directory).
Any path with a lib/ directory should be a package: URI instead.
Any path inside the same lib/ directory should be relative.
(And always use lower-case only file and directory names, even on Windows).

The usually exception comes from Flutter encouraging people to put a "main" library into lib/, so you have to run it using flutter lib/main.dart - which is a path containing lib/!
We have fixed that with a hack in the parser, recognizing that an entry point inside lib/ should probably be treated as package:my_package/main.dart instead, and warning the user that we are doing that.

[eernstg]

FYI: @lrhn already created this issue, proposing that a certain amount of implementation specific normalization of URIs should be applied. This would help us avoid the situation where the same library (in some sense, e.g., same absolute path on a traditional UN*X style file system) is imported multiple times using different URIs.

It would still be possible to create a similar problem (say, using several hard links), and it would be possible to go further and decide that textually identical libraries are "the same" library, but we can't hope for a complete and perfect solution. So a modest amount of normalization as proposed by @lrhn is probably the best approach.

[escamoteur]

@mit-mit the verbosity of the package notation isn't a problem with the autoimport and path completion of the IDEs

[escamoteur]

@lrhn what is the benefit to use relative urls inside lib? Why should we prefer them there?

[mit-mit]

the verbosity of the package notation isn't a problem with the autoimport and path completion of the IDEs

I (subjectively) disagree; I find it a lot more verbose to read.

[escamoteur]

@mit-mit that's another point, where I agree. if we can somehow assure that either mixed imports still only create one library instance or give a warning if this is detected I'm also fine with.

[mit-mit]

...or give a warning if this is detected...

Is that not suggestion #3 in https://github.com/dart-lang/sdk/issues/41868#issuecomment-627941775 ?

[lrhn]

@lrhn what is the benefit to use relative urls inside lib? Why should we prefer them there?

Brevity.

You also avoid hard-coding your own package's name. If you want to change the name of the package, which does happen during development, you only need to change the name entry in pubspec.yaml. (Well, and any imports in test/ and bin/, but nothing in lib/ where the majority of your code presumably is).

[escamoteur]

Yep that would be an option although currently not every project you create with flutter create has an analysis_option included

But having both ways work smoothly together would definitely the best solution

[mit-mit]

Yes, we would fix that as part of option 3 (we're already working on a new minimal set of "must do" lints we're planning to add per default to all Flutter & Dart templates).