dart-lang / language

Design of the Dart language
Other
2.61k stars 200 forks source link

Are import shorthands worth the cost in readability/cognitive load? #1941

Open leafpetersen opened 2 years ago

leafpetersen commented 2 years ago

Reading the import shorthand proposal, I am struck at the level of complexity for a user that this is introducing. Consider this text:


Examples:

- `import built_value;` means `import "package:built_value/built_value.dart";`
- `import built_value:serializer;` means `import "package:built_value/serializer.dart";`.
- `import :src/int_serializer;` means `import "package:built_value/src/int_serializer.dart";` when it occurs in the previous `serializer.dart` library, or anywhere else in the same Pub package.
- `import ./src/int_serializer;` means `import "./src/int_serializer.dart"`, aka.`import "src/int_serializer.dart"`, when it occurs inside the previous `serializer.dart` library.
- `import ../serializer;` means `import "../serializer.dart"` when it occurs inside the previous `src/int_serializer.dart` library.

This is an extraordinary amount of new cognitive load for a user. There are now somewhere around 8 ways to write an import of serializer.dart, some of which are only valid in certain locations, and all of which use overlapping syntax to mean different things (":something means the package part was elided, but foo:something means add the package: back in, change the : to a / and add a .dart"). There's numerous ways to write the same thing:foo and foo:foo and package:foo/foo.dart and :./foo and .foo and "./foo.dart" and "foo.dart" all might mean the same thing (I think? I'm a little lost in the weeds.).

Is this really worth the shorter syntax? I really worry that we're making code significantly less consistent and readable in the name of brevity.

cc @lrhn @munificent @eernstg @jakemac53 @natebosch @bwilkerson

jakemac53 commented 2 years ago

Note that pub run also already has a meaning for built_value:serializer which is different, it means the serializer executable in the built_value package, which would usually be at bin/serializer.dart.

If we shipped this feature with the : syntax it might be very confusing for users if they do dart run foo:bar, they may expect it to mean package:foo/bar.dart.

srawlins commented 2 years ago

I also worry about using : as the delimiter between a package name and a top-level library in that package (import build_value:serializer), as it is already a delimiter in the quoted syntax, in, e.g. "package: and "dart: (and I don't think others? It doesn't accept "file://, right? You can't reference Windows driver letters, right?)

I'm sure just about any other delimiter looks weird too though.

srawlins commented 2 years ago

Perhaps something that contributes to confusion is that the new syntax (without quotes) is not distinguished from the old (with quotes) by a different token/delimiter. It is only different in that tokens (the quotes) are omitted. Adding a new (leading?) token may look strange, but catches the eye and makes very clear that a new style is in play. import @built_value/serializer or something similar.

I think the quotes are characters that the brain doesn't really catch on. I'd have to look twice to see whether an import directive is using them or not.

Levi-Lesches commented 2 years ago

I actually find it's simple to understand, but it would really simplify things if we kept package imports and file imports seperate: package imports use the new proposal, and file imports use the old quoted syntax.

// Assuming we are in lib/src/logic.dart of a package called my_package
import built_value;             // 1. "package:built_value/built_value.dart"
import built_value:serializer;  // 2. "package:built_value/serializer.dart"
import :data;                   // 3. "package:my_package/data.dart"
import "../data.dart";          // 4. Matches current behavior, imports lib/main.dart like #3
import "firestore.dart";        // 5. Matches current behavior, imports lib/src/firestore.dart
import "data/user.dart";        // 6. Matches current behavior, imports lib/src/data/user.dart

This makes sense using the following rules:

Technically, I suppose that means import :; should import "package:my_package/my_package.dart";, but that should be disallowed/discouraged because it looks terrible (I've never even had to do that before). But besides for that, I think limiting the syntax to a:b for package imports and quoted strings for file imports makes the most sense, both matching what users are already familiar with while intuitively shortening some of the longer imports.

EDIT: Regarding the difference between dart run a:b and import a:b, I think the distinction is quite intuitive. In both cases, users are saying the same thing: Give me the code b from package a. In the case of dart run that means in bin/ and in the case of import that means in lib/, but those details aren't important. Nobody "runs" a lib/ file, and nobody imports a bin/ file, so Dart can choose the right one by itself and not bother the user with making that choice.

natebosch commented 2 years ago

I do worry about overloading the package:binary from dart run with package:library for an import, but I'm not worried about the rest.

I'm not worried about making code less readable - I think we're stripping away noise which should make it more readable. It's a high cognitive load to answer the questions

I'm not worried about introducing too much choice for users - I think we can, and should, provide a single recommended pattern to use. I think the only "new" choices we are introducing are whether to use the old syntax or new (I think we recommend to use the new syntax) and whether to use package:path or :path for same-package imports (I think we recommend to use the latter).

The choice that's harder for us to give a universal recommendation on is package vs relative imports in the same package - and that choice is not changing with this proposal.

I don't think the new syntax will have a higher cognitive load in the long run. It's a high cognitive load to think about exactly how to translate to the old syntax, but not necessarily a high cognitive load to understand what library the syntax represents.

TimWhiting commented 2 years ago

I personally don't think it is a high cognitive load, since the difference between package / relative imports already exists, the only change is that this gets rid of some of the noise. The only concern I see is the pub run syntax.

However, pub run already has a subcommand sort of syntax (though I guess this is probably just an argument parser in the executable) for example

pub run build_runner build

Why not deprecate the pub run package:executable syntax and encourage the subcommand syntax instead of building the language around the external tool? It is only a visual conflict, and shouldn't affect anyone during the migration period since dart run is what looks in the lib folder, and pub run looks in executable folders.

rrousselGit commented 2 years ago

Personally, I don't see the value in the proposal.

With IDE integration, I neither need to read nor need to edit imports. The IDE will automatically add/remove/sort them for me.

Even when there is a name conflict (which is rare), it's not the imports that I will look at, but the compiler error message which says "Class A is defined in both package_a and package_b".

The only thing I really care about is exports. But exports primarily use relative file paths.

munificent commented 2 years ago

Is this really worth the shorter syntax?

Yes, I strongly believe it is. From the day we introduced pub to Dart, I have wanted a better import syntax. It's always been bad. Here's how you idiomatically import SomeUsefulThing in various languages:

import SomeUsefulThing                                      // Swift
import org.cool.SomeUsefulThing                             // Kotlin
import org.cool.SomeUsefulThing;                            // Java
using CoolOrg.SomeUsefulThing;                              // C#
import { SomeUsefulThing } from "./SomeUsefulThing";        // TS/JS
import some_useful_thing                                    # Python
require 'some_useful_thing'                                 # Ruby

Here's Dart current and proposed:

import 'package:some_useful_thing/some_useful_thing.dart';  // Dart now
import some_useful_thing;                                   // Dart shorthand

Our current syntax is really verbose, even worse than JavaScript, which is saying something. We are the only language that requires a file extension in every import (!). We're the only one with essentially two keywords (import and package) that you have to write when importing from a package. Most don't require quotation characters. In the common case of a package's name being the same as its main library, you have to say the same name twice. It's bad.

I really worry that we're making code significantly less consistent and readable in the name of brevity.

The inconsistency will be temporary and a fix is easily automatable. It would be less inconsistent if we'd designed a good syntax to begin with, but here we are.

I don't think it's less readable. I think having to squint past the boilerplate "package:___.dart" to find the actual thing being imported is harder to read.

This is an extraordinary amount of new cognitive load for a user. There are now somewhere around 8 ways to write an import of serializer.dart, some of which are only valid in certain locations,

Don't forget ./serializer.dart, ././serializer.dart, ../../../back/into/dir/serializer.dart, etc. :)

I agree there are a lot of potentially different ways to refer to libraries but for any given library the guidance is pretty simple. It's:

  1. If you're importing a library in another package:

    1. If the library has the same name as the package, use import name;.
    2. Else, use import package_name:dir/name;.
  2. Else, if you are importing a library with the same top-level directory (i.e. you are not reaching into or out of lib, bin, etc.), use import ./relative/path;

  3. Else, use import :dir/name;

Compare that to the current rules:

  1. If you're importing a library in another package, use import 'package:package_name/dir/name.dart';.

  2. Else, if you are importing a library with the same top-level directory (i.e. you are not reaching into or out of lib, bin, etc.), use import './relative/path.dart';

  3. Else, use import '/dir/name.dart';

The only additional complexity is the shorthand for a package whose name is the same as the library, but that rule also avoids a very common source of redundancy.

Users will have to be aware of both the old and new forms during the migration, which is a drag. I think it's worth the transition.

and all of which use overlapping syntax to mean different things (":something means the package part was elided, but foo:something means add the package: back in, change the : to a / and add a .dart").

I think it's simpler to understand the semantics directly and not in terms of a desugaring to the old crappy syntax:

  1. If the import starts with ./ or ../, then it imports the .dart library at that path relative to the current library.
  2. If it's of the form foo:some/path, then it imports some/path.dart from package foo. (There also happens to be a magical built-in package named dart.)
  3. If it's of the form :some/path, then it imports some/path.dart from the current package.
  4. If it's of the form foo, then it imports foo.dart from package foo.

(There is also a special rule for handling dotted package names, but that's only meaningful for internal users.)

I'm personally not too attached to (3), and would be OK with ditching the :foo syntax.

There's numerous ways to write the same thing:foo and foo:foo and package:foo/foo.dart

There's numerous ways to create a list of integers too, but as long as there's a clearly better idiomatic one, it doesn't seem too onerous to me.

and :./foo and .foo and "./foo.dart" and "foo.dart" all might mean the same thing (I think? I'm a little lost in the weeds.).

Only the last two of these are valid, and those are both valid today.

leafpetersen commented 2 years ago

Here's how you idiomatically import SomeUsefulThing in various languages:

import SomeUsefulThing                                      // Swift
import org.cool.SomeUsefulThing                             // Kotlin
import org.cool.SomeUsefulThing;                            // Java
using CoolOrg.SomeUsefulThing;                              // C#
import { SomeUsefulThing } from "./SomeUsefulThing";        // TS/JS
import some_useful_thing                                    # Python
require 'some_useful_thing'                                 # Ruby

How many other ways are there to import things in each of those languages?

TimWhiting commented 2 years ago

How much breakage would happen if the old import syntax was deprecated then later removed so that there is only one way to import?

What invalid characters are people using that would cause issues?

Levi-Lesches commented 2 years ago

How about leaving quotes in as the default for relative imports? That way, invalid characters can still be supported and there would only be one way of doing things: package imports use package:path and relative imports use quotes, instead of 2 ways of doing both (it doesn't seem like relative and package imports are being merged anytime soon).

From my earlier comment, you'd have this very concise list of options:

// Assuming we are in lib/src/data/user.dart of a package called my_package
import built_value;             // 1. "package:built_value/built_value.dart"
import built_value:serializer;  // 2. "package:built_value/serializer.dart"
import :data;                   // 3. "package:my_package/data.dart"
import "profile.dart";          // 4. Matches current behavior, imports lib/src/data/profile.dart
import "../firestore.dart";     // 5. Matches current behavior, imports lib/src/firestore.dart
cedvdb commented 2 years ago

Anyone should be familiar with those 3 types of imports:

Those 2 are questionable, to me at least, as it's not immediately obvious what the : means:

The : could have a single meaning of external dependency instead:

lrhn commented 2 years ago

We can definitely ditch the :foo syntax and use /foo instead. It would mean the same thing, equivalent to "/foo.dart" today. That makes anything starting with /, ./ or ../ a path-only reference relative to the current location/package, and anything containing id: refers to a package.

I was once convinced by others that :foo was more understandable than /foo (don't remember why any more), but obviously opinions differ.

As for why not build_value/serializer: I actually want to make the package name distinguishable from the path. If I'm inside "package:build_value/serializer.dart", known by the path build_value/serializer, and I do ../../collection/collection, what does that mean? (Answer: By the URI rules it means package:build_value/collection/collection.dart, because the package name is not part of the hierarchical path, it's an outside selector more like if the original URI had been package://build_value/serializer.dart, and that's how the Dart URI class treats package URIs). So, I prefer to not use / for something which is not part of the path. Putting something significant in front of the package name is another option, like //package_name or @package_name, but it's more verbose that just package_name and package_name:path.

lrhn commented 2 years ago

About the cognitive load, I think people will quickly learn the basic rules:

If you omit the quotes, then the import must start with one of:

  • a package name (when importing from another package),
  • dart: (importing a platform library), or
  • one of /, ./ or ../ (path importing from the same package).

If the library you import from another package is not the main library (same name as package), you use package-name:library-path.

(For experts only: Package names containing .s.)

(Switched :path to /path here. Cognitive-load-wise, I think it fits better with ./ and ../ as a single case).

lrhn commented 2 years ago

Technically, the Dart language allows any URI as import. Only package: and dart: URIs are guaranteed to work, because they are actually part of the language and mentioned in the language specification.

Our tools (compilers included) support loading files from other URI schemas than those two, currently probably only file:, http: and https: (I haven't actually checked).

The import shorthand will only apply to package:, dart: and relative paths. That should cover 99.999% of actual imports. It still resolves to an actual URL before being loaded, it's just a shorthand for some (very vast majority) of URIs.

It also just applies to paths containing only ASCII letters, digits, dots and underscores, which again applies to almost all actual imports (I'm open to adding - to the list if necessary).

lrhn commented 2 years ago

The compiler will know exactly waht to do.

If you write import http:http; it's the same as import http; and import "package:http/http.dart";.

Import shorthands, without quotes, cannot specify anything other than package: URIs, dart: URIs, or relative paths. If you need a file: or http: schemed-URI (which is not just relative to the current library's URI), you need to use the "old-style" quoted syntax.

Unless you're writing stand-alone scripts, with no surrounding Pub package, that won't happen. Even those will likely use relative imports 99% of the time.

cedvdb commented 2 years ago

As for why not build_value/serializer: I actually want to make the package name distinguishable from the path.

I'm confused. Isn't the package name the first value in the path (here built_value) , or is it possible for that not to be the case ? I assumed the the current way of importing a local file from the same directory would need a ./ in front from here on out.

We also know it's a package because it does not start with one of: ./, /, ../.

If I'm inside "package:build_value/serializer.dart", known by the path "build_value/serializer", and I do ../../collection/collection, what does that mean?

What does "I'm inside" and "I do" mean in this case ? I parse this as "I write import ../../collection/collection in the build_value/serializer.dart file" which makes no sens as it's going outside the lib.

lrhn commented 2 years ago

@cedvdb I agree that it makes no sense and you are going outside "the lib directory", but there is no "lib" visible in what looks like a path. That's why I prefer to separate the package name from the hierarchical path with a character other than /, because it's not like the other /s in the path.

It can certainly work to use /. The syntax is unambiguous.

You do import build_value/serializer; or import foo/bar/baz;, but if, in the latter library, doing an import ../../../qux will ... either be an error (my preference) or just mean import foo/qux;. The first slash is special, it's not like the other slashes, but it doesn't look special. That's what I want to avoid by using : instead. (That, and making dart:async stay the same as now, while following the same syntactic pattern, looking like it's a "dart" package, but that's just a bonus.)

lrhn commented 2 years ago

@tatumizer You do know to push where it hurts, don't you :stuck_out_tongue_closed_eyes:

Yes, that one annoys me too. It looks like it should be allowed. It's not, it is an error.

It's currently not allowed grammatically. The grammar is very restrictive and says something like (from memory):

importShorthand ::= packageName (: path)? | relativePath path ::= pathSegment (/ pathSegments)* relativePath ::= ./ path|../+path|/`path

It's not a fully general "anything goes" grammar. You can't have embedded /./ or /../ path segments (only one leading ./ or / or a number of leading ../s), no adjacent //s, etc. And no packageName:/path.

I do consider import /foo; a very likely alternative to import :foo;. (I've been going back and forth between the two, leaning towards / today).

leafpetersen commented 2 years ago

I do consider import /foo; a very likely alternative to import :foo;. (I've been going back and forth between the two, leaning towards / today).

FWIW this feels less confusing to me as well. If I understand it correctly, with that model I really only need to think about two kinds of abbreviations:

Technically, there's still the old syntax for things that require escaping, but that's ok, you only use those for things that require escaping.

Is that the intuition for dummies like me?

lrhn commented 2 years ago

@leafpetersen No, that sounds exactly right. And you can write import dart:async; as if dart was a package.

(Or you can think of it as separate, but similar syntax for platform libraries only. In the long run, people will probably start to think of dart as an implicitly available package, and that's fine with me.)

lrhn commented 2 years ago

Now I remember the reason for preferring : as the "current package" identifier: It can be made to work for files in test/ and bin/ of a Pub package too without being weird.

So, if import /path is just equivalent to import "/path.dart", and import :path is equivalent to import current_package:path, then inside lib/ (in a file with a package: URI), they work the same.

Then :path only works on files "inside a package", but /path "works" everywhere (in that it has a well-defined meaning, just not one you'd ever want to use.)

Outside of lib/, the :path works, and is meaningful when writing code in test/ and bin/, and /path probably doesn't work for anything. You likely have a file: URI for the surrounding file, and you are very, very unlikely to want to specify an import starting from the root of your file system. You'll always want to use relative paths for that.

We can allow both syntaxes, but that risks someone using /path in bin/ instead of :path, because the difference is so subtle. That's bad. Then we can disallow using / in non-package: files. That's probably a good idea, but reduces its power significantly. That makes the :path functionality a strict superset of /path, they do the same inside lib/, which is the only place a /path import is allowed, and :path works in the rest of the Pub package as well.

That suggests just having one of them, which again points to :path as the more general one. Or allowing both, and recommend using /path inside lib/ and :path outside of lib/ (but allowing :path inside lib/ as well, if you want to.)

Really, /path has nothing going for it except that it reads better as a path instead of a package reference.

I'd like to be opinionated with this design (if you do something non-idiomatic, use a string URI instead), and it irks me to have two ways to do the same thing. And it also irks me to use :path where /path seems more right (to me, which might be the problem: it's heavily subjective).

WDYT?

jakemac53 commented 2 years ago

I think it would be very weird if / referred to the package uri root (usually lib, but it doesn't have to be!). I would be sort of ok with it referring to the actual root of the package (ie: next to the pubspec.yaml), but that is a lot less useful also. I think it is probably best to just avoid it.

jakemac53 commented 2 years ago

Do you mean that the situation is less weird with import 'package:built_value/serializer.dart'; than it could be with simply import "/serializer.dart" ? To me, it looks like the same thing.

Yes, to me / very clearly means either the actual root of the package (ie: where the pubspec lives), not the "package uri root" ie: where package: uris resolve to. It would be highly confusing if it resolved to the package uri root to me.

Hixie commented 2 years ago

FWIW, I 100% agree with the concerns that there's too much complexity here.

I would go with only four forms (two of which exist today):

import '../relative/path.dart'; // relative paths are strings and this works fine today
import 'package:foo/bar.dart'; // package imports can give precise paths and work fine today
import foo; // shorthand for importing "package:foo/foo.dart", most intuitive syntax, matches other languages
import foo.bar; // shorthand for importing "package:foo/bar.dart", matches other dot-delimited features in Dart

The non-quoted forms would be either or ., nothing more complicated. This makes parsing it trivial and unambiguous to readers, and avoids introducing yet another kind of token for people to understand.

natebosch commented 2 years ago

. should not be the separator. It is a valid character in a package name (not in pub) and it's how we map to hierarchical packages in bazel.

Hixie commented 2 years ago

I think . has to be the separator if we have one (we don't have to have one, we could just not support importing libraries in this way with the new syntax).

We shouldn't let bazel affect how we design the language, IMHO.

Hixie commented 2 years ago

(All the popular languages that support a way to do this use periods, none of them use colons. I think that's an overwhelming argument in favour of periods.)

munificent commented 2 years ago
  • "Path imports". Either "absolute" (.e.g using import /foo) or relative (import ../foo)
  • "Package imports". General form is somepackage:filepath but can just use somepackage as a shorthand for somepackage:somepackage.

I think a better mental model is that leading "/" or ":" (whichever syntax we choose) are package imports, not path ones. So it's basically:

munificent commented 2 years ago

I think . has to be the separator if we have one (we don't have to have one, we could just not support importing libraries in this way with the new syntax).

I am 100% with you aesthetically, but the consequence of that would mean designing a syntax that literally none of our Googler users would be able to take advantage of with their thousands and thousands of imports, many of which are incredibly long and really would benefit from a shorthand because they use these long dotted package names.

You're right that other languages tend to use . for path separators in imports, but Dart has already taught users to use / in import path separators, so it's not like we're introducing something new now. All we're doing is removing the quotes and the .dart.

Hixie commented 2 years ago

There are 1000s of Dart users at Google. There are 100,000s of Dart users outside Google, potentially millions. If we want Dart to be used in the wider world, we cannot let the esoteric needs of our colleagues outweigh the needs of the entire world.

munificent commented 2 years ago

There are 1000s of Dart users at Google. There are 100,000s of Dart users outside Google, potentially millions. If we want Dart to be used in the wider world, we cannot let the esoteric needs of our colleagues outweigh the needs of the entire world.

Given the choice between a syntax that benefits 100,000s of users versus one that benefits 100,000s plus 1,000s, why wouldn't we take the latter? It's not like / is harmful—it's the syntax they already use for path separators.

Hixie commented 2 years ago

IMHO having an unquoted string containing a slash is unintuitive. It looks like a path separator, there's nothing in Dart that allows this syntax today, it's not used by any major language. Basically it fails the simplest test as to whether something is a good feature for Dart: is it boring? It's not boring. It's something we'd have to teach people because we would be arbitrarily (to their eyes) different than everyone else.

leafpetersen commented 2 years ago

I think a better mental model is that leading "/" or ":" (whichever syntax we choose) are package imports, not path ones. So it's basically:

  • Path imports. Always relative starting with ../ or ./.
  • Package imports. General form is somepackage:filepath, but can just use somepackage as shorthand for somepackage:somepackage, and can omit somepackage if it's the current package. (But can't apply both shorthands because import :; is hilariously weird.)

Hmm. This makes me slightly more nervous about the / over :. It feels weird that ./foo is a package URI but /foo is not. That feels like a footgun: /src/foo and ../foo will potentially result in different URIs and hence incompatible types? Do I have that right?

leafpetersen commented 2 years ago

IMHO having an unquoted string containing a slash is unintuitive. It looks like a path separator, there's nothing in Dart that allows this syntax today, it's not used by any major language.

It may just be my unix background showing through, but this doesn't bother me at all. cd ./foo/bar works just fine, why not import ./foo/bar. And I do like that it's mostly consistent with the "quoted syntax" (that is, the current import syntax). It feels a bit off to me to have the quoted syntax and the shorthand syntax use different path separators.

On the gripping hand, maybe having completely different syntax between the two is valuable. In the simplified proposal from @Hixie , all shorthands are package URIs, so there's no confusion about whether the shorthand syntax means package or relative.

leafpetersen commented 2 years ago

. should not be the separator. It is a valid character in a package name (not in pub) and it's how we map to hierarchical packages in bazel.

@natebosch

How do other languages that do use . as a separator deal with this in bazel?

An assuming there's a way to handle this is there a path we could take where we do a large scale change to a new syntax internally?

TimWhiting commented 2 years ago

I dislike the idea of having to mix the old and new syntax. To me it is not worth the effort unless all reasonable imports can be written the new way; accepting the fact that you might have to rename some files to remove special characters that aren't allowed in the new syntax. A mix of old and new syntax just makes the language look inconsistent and confusing.

Hixie commented 2 years ago

A mix of old and new syntax just makes the language look inconsistent and confusing.

I agree. The exception to this I think is that relative paths being strings looks ok combined with foo.bar to import packages, because it makes a clear distinction between "implementation details" and "imported public APIs":

// dart libraries
import dart.io;

// packages
import flutter.services;
import flutter.material;
import provider;

// local files
import 'basic_types.dart';
import 'widgets/slider.dart';

I like this because it makes string imports feel more like something you're building. Like, obviously basic_types.dart isn't part of the language, since you just made it up, so naturally it should be a string.

As a corollary, it also makes hacks like importing files from inside packages look like a hack:

import foo;                            // this is a fine import
import foo.bar;                        // so's this
import 'packages:foo/src/baz.dart';    // woah, what is this though
TimWhiting commented 2 years ago

I'll agree to disagree, but I would prefer not to have two forms, even for local imports. The quotes and .dart suffix are entirely extraneous. However I like the package / dart imports using the dot notation and local files using the path notation.

// dart libraries
import dart.io;

// packages
import flutter.services;
import flutter.material;
import provider;

// local files
import ./basic_types;
import ./widgets/slider;

You still get the benefit of src imports from other packages looking strange:

import foo;                       // this is a fine import
import foo.bar;                  // so's this
import foo.src/baz;              // woah, what is this though (package:foo/src/baz.dart)
Hixie commented 2 years ago

The bare path syntax doesn't match anything else in Dart. This means it doesn't reuse any of the knowledge people have from Dart, so it's an extra cognitive load. It also doesn't match anything in any other mainstream language. As proposed it's not even a path, so it doesn't even match bare paths in Bash.

Also, it's not the same as the other syntax, so it's not actually simplifying the language down to one form, it's still two forms. Three, really. <ident>, <ident>.<ident>, and ./<path-like> (or something? I'm not sure what foo.src/baz means.)

Indeed, because it's not actually a path (paths can have spaces and newlines and such, and periods, not to mention the extension), we'd still actually need the string syntax to handle cases it couldn't handle. So we'd actually have four or more forms depending on exactly what is being proposed. I stand by my earlier proposal as being simpler.

cedvdb commented 2 years ago

I would not forget about absolute path imports as those are useful when the project is well organized at the root but tend to have a deeply nested folder structure with barrels at each level. In that case using absolute paths is easier.

/folder/file is familiar to people who have done web development for instance.

Although the dot might be common in some other languages, it makes it harder to have all the pieces fit together without mixing dots and slashes. ../, ./ use the dot but this is familiar to anyone.

paths can have spaces and newlines and such

It is generally accepted, as an unwritten rule, that those should not be part of file paths during development. This seems like a reasonable decision to not allow that. If such an import does not work it should be obvious why.

and periods

Some framework (angular) actively encourage using a semantic extension .service.ts, .component.ts which uses the dot syntax. Personally this type of semantic extension is what I use in my dart projects too.

lrhn commented 2 years ago

Some Dart files do have embedded .s, like generated files named foo.g.dart, so using . as "path separator" means those files cannot be named, and must fall back on quoted string syntax.

I'd prefer to not go there, and that's why I prefer / as separator. Also, I think it reads perfectly well, and using .s is confusing when compared with other places where Dart uses dotted names (like library names).

The syntax shorthand is tailored to allow existing Dart file/directory and package names, including Bazel packages with dots in them. That's why I'll add - to the allowed characters, because there are directories with that character in them.

As for / being relative to the Pub package root, not the Dart package root, that's a strong no from me. I can see it might be useful, but it is too risky. We never want a file inside lib/ to refer outside of lib/, so we would have to make /foo invalid inside lib/. Having to disallow the syntax inside the main code location of a Pub package is disqualifying for that syntax. And then we'd still need a "root of Dart package" shorthand, like :foo.

We'd also never want something outside of lib/ to use /lib/foo to refer into lib/, so we'd have to disallow that too.

lrhn commented 2 years ago

@tatumizer

leave the syntax as it is now, just remove quotes and .dart suffix

The "syntax as it is now" is full URI syntax + implicit URI normalization + Dart string literal syntax. I can write:

import """
\x61bc.dart
import bar""";

and it's the same as import "abc.dart%0aimport%20bar";.

You can create a file with that name (at least on Unix, not sure about Windows allowing newlines in file names), and it doesn't have to end with .dart to begin with.

Just leaving off the quotes and the .dart isn't viable. We need a restricted syntax in order to be able to know where the URL is delimited.

If we restrict it and only allow "current syntax" when it's is [a-zA-Z0-9_-]+ words with / separators and possibly leading ../s, then it would mean that import foo; would be a local import, and we really, really want to use that for the most used package import, so you don't have to repeat the package name for import test:test;.

Only supporting top-level imports from other packages is an interesting restriction. Not perfect, because some packages can have sub-directories other than src that you are allowed to import from. For example package:analyzer/dart/.... So, not sufficient to support existing imports. (RegExp used to search: import\ ['"]package:\w+/([^s]\w*/|s[^r]\w*/|sr[^c]\w*|src\w+/), sadly no negative look-ahead where I searched.).

cedvdb commented 2 years ago

It's hard to follow the discussion here as everyone is trying to get involved with a slightly different syntax. It would be nice to have a summary of where this is heading.

Levi-Lesches commented 2 years ago

I'll vote in favor of @Hixie's proposal, because it restricts the new unquoted form for the "ideal" package imports, and uses the existing quoted form for relative imports and anything else, while still allowing existing code to work.

munificent commented 2 years ago

I agree it's getting hard to follow. In the original discussion, I offered up this representative set of imports:

import 'dart:isolate';
import 'package:flutter_test/flutter_test.dart';
import 'package:path/path.dart';
import 'package:flutter/material.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:widget.tla.server/server.dart';
import 'package:widget.tla.proto/client/component.dart';
import 'test_utils.dart';
import '../util/dart_type_utilities.dart';
import '../../../room/member/membership.dart';
import 'src/assets/source_stylesheet.dart';
import 'user_address.g.dart';

(I added one more to the list to cover the funny file extensions with generated files.

I believe that covers all of the interesting cases. If you're suggesting a change to Lasse's proposal, it would help to show what your proposed syntax would look like for all of those (even for some cases your proposal is "keep using the current syntax").

The current proposal is:

import dart:isolate;
import flutter_test;
import path;
import flutter:material;
import analyzer:dart/ast/visitor;
import widget.tla.server;
import widget.tla.proto:client/component;
import ./test_utils;
import ../util/dart_type_utilities;
import ../../../room/member/membership;
import ./src/assets/source_stylesheet;
import ./user_address.g;

As far as I can tell, there are two import styles in wide use:

(Python is an outlier, which uses a dotted syntax but maps that to file paths.)

If you think of Dart libraries as denoting a hierarchy of logical modules, then it makes sense to want an unquoted dotted form to import them. But inside Google, we already have a logical dotted hierarchy of package names, so we'd need some way to distinguish those two hierarchies. If you think of libraries as files, then a quoted slash-separated path is a better fit.

Personally, my goal is simply to eliminate pointless text. That means:

  1. Not having to write the package name twice in imports like package:flutter_test/flutter_test.dart.
  2. Not having to write .dart. You're importing a Dart library from a Dart library. Of course the path ends in .dart.
  3. Something shorter than package: to distinguish package imports from relative imports.
  4. Getting rid of the two ' characters. I care less about this since it's just two characters.

That's really it. This is entirely a verbosity feature for me. I'll be fairly happy with any proposal that eliminates as much of those for as many imports as possible.

Lasse's proposal addresses all four points. I don't find @Hixie and @TimWhiting's proposals compelling because they don't apply to any package imports inside Google. Eliminating a lot of characters from a smaller fraction of imports is not a big win for me. I want to make as many imports better as possible. @Hixie's proposal also doesn't eliminate redundant .dart from relative imports.

Hixie commented 2 years ago

@lrhn:

Some Dart files do have embedded .s, like generated files named foo.g.dart, so using . as "path separator" means those files cannot be named, and must fall back on quoted string syntax.

That's perfectly fine, though, because those files are never imported from other packages. I see no problem with using strings for the current package.

@munificent:

I agree it's getting hard to follow. In the original discussion, I offered up this representative set of imports:

This is a good list. It separates nearly into three categories: first, those that are what Levi calls "ideal" package imports:

import 'dart:isolate';
import 'package:flutter_test/flutter_test.dart';
import 'package:path/path.dart';
import 'package:flutter/material.dart';

Second, those that are things we should discourage, e.g. because they use non-idiomative syntax, encourage deep links into packages, or generally do things that are not "ideal" and therefore should stand out so that people are more likely to avoid them:

import 'package:analyzer/dart/ast/visitor.dart';
import 'package:widget.tla.server/server.dart';
import 'package:widget.tla.proto/client/component.dart';

...and finally third, local imports:

import 'test_utils.dart';
import '../util/dart_type_utilities.dart';
import '../../../room/member/membership.dart';
import 'src/assets/source_stylesheet.dart';
import 'user_address.g.dart';

IMHO the last set is not a problem at all: there's no verbosity here; you're specifying a file name. The use of string and explicit extensions is actually a good thing because it makes the reader 100% clear on what is happening. It's consistent with many other language's local imports.

The first set are the ones where verbosity is most problematic. It's also the set with the least controversy over how to represent the results.

The second set are all examples of things I would try to discourage by not providing a convenient syntax. Using periods in package names is dubious at best, and violates best practices (documented both in the pubspec guidelines and the dart style guide, there's even a lint for it). Doing deep imports is frowned upon, and for good reason: it makes the package API very confusing (and the API docs, too, e.g. try looking up visitor in the analyzer API docs).

The current proposal is:

I would say "A" current proposal. There have been many.

As far as I can tell, there are two import styles in wide use:

  • Unquoted dotted names import from a logical namespace or module hierarchy (Java, Kotlin, C#, Swift). The name doesn't refer to a file path, it refers to a "module" in some abstract hierarchy of names. The mapping of modules to files is handled outside of the language (for example in csproj files for C# or the class loader for Java).
  • Quoted slash-separated paths import from files or URLs (C, C++, CSS, Go, JavaScript, Lua, PHP, Ruby, TypeScript). The path is a string literal and it imports from a concrete location. Most "scripting" languages work this way.

(Python is an outlier, which uses a dotted syntax but maps that to file paths.)

This seems like a fine pair of syntaxes to support. (I question separating Python from the first set, though.)

But inside Google, we already have a logical dotted hierarchy of package names, so we'd need some way to distinguish those two hierarchies.

Google is a small fraction of the overall Dart usage in the world, even smaller when you think of Dart in 10+ years if we continue to make the language appealing to people. Making choices here that are less than optimal for the world at large in order to optimize for today's Google is illogical, all the more so given that Google is the easiest codebase for us to change.

Personally, my goal is simply to eliminate pointless text.

I agree that the only real problem with Dart imports today is their verbosity.

  1. Not having to write the package name twice in imports like package:flutter_test/flutter.dart.
  2. Not having to write .dart. You're importing a Dart library from a Dart library. Of course the path ends in .dart.
  3. Something shorter than package: to distinguish package imports from relative imports.
  4. Getting rid of the two ' characters. I care less about this since it's just two characters.

These seem like reasonable goals, though I think #2 only makes sense in the specific case where you're importing something that isn't conceptually a file (e.g. when you're importing a package), and I see no real value in #4, especially if it would only come at a cost of extra confusion (as in the proposals above).

Lasse's proposal addresses all four points.

But has other downsides, as previously discussed.

I don't find @Hixie and @TimWhiting's proposals compelling because they don't apply to any package imports inside Google.

I don't see why that matters.

Eliminating a lot of characters from a smaller fraction of imports is not a big win for me. I want to make as many imports better as possible.

I disagree with this goal. I think it's actually good to make some imports "worse" than others, because it will encourage more idiomatic and cleaner, more consistent, less confusing, practices across the ecosystem.

@Hixie's proposal also doesn't eliminate redundant .dart from relative imports.

I think that's actually a positive, because it prevents any confusion about what you're importing.

I bet that if we do usability studies on this (which we should), people will have no trouble identifying what is imported by these:

   import dart.math;
   import intl;
   import printing.printing_web;
   import '../foo/bar.dart';
   import 'package:widget.tla.server/server.dart';
   import './foo.g.dart';

...but they will struggle to say what is imported by these:

   import widget.tla.server/server;
   import ./foo.g
munificent commented 2 years ago

Second, those that are things we should discourage, e.g. because they use non-idiomative syntax, encourage deep links into packages, or generally do things that are not "ideal" and therefore should stand out so that people are more likely to avoid them:

import 'package:analyzer/dart/ast/visitor.dart';
import 'package:widget.tla.server/server.dart';
import 'package:widget.tla.proto/client/component.dart';

No, there's nothing non-ideal about these. These are all perfectly idiomatic, valid imports. There's no reason for a user writing any of these imports to feel bad, and if the import syntax did make them feel bad, there's nothing they can do to fix it. We should not give users syntax deliberately bad in order to punish them for something they have no control over. We certainly shouldn't punish them for writing code that is idiomatic and recommended, which all of these imports are.

Google is a small fraction of the overall Dart usage in the world, even smaller when you think of Dart in 10+ years if we continue to make the language appealing to people. Making choices here that are less than optimal for the world at large in order to optimize for today's Google is illogical, all the more so given that Google is the easiest codebase for us to change.

Comparing your proposal to Lasse's, I see:

  1. Lasse's proposal applies to more imports and more users.
  2. Yours has greater aesthetic appeal to you and possibly more users.

It's hard to compare these because they are being judged by unrelated metrics. The former is an objective numeric fact—we could theoretically write a program to count the number of lines affected by both proposals and Lasse's could come up greater. The latter is a subjective preference.

Both axes matter, and I don't see how either can be disregarded out of hand. Users are users whether they work at Google or not, and the more users we can benefit, the better the proposal. Obviously, we shouldn't cater to Googlers at the expense of other users, but I haven't seen anything to suggest that Lasse's proposal does that.

@Hixie's proposal also doesn't eliminate redundant .dart from relative imports.

I think that's actually a positive, because it prevents any confusion about what you're importing.

Is there confusion that an import in a Dart library is going to import a Dart file? It's hard to imagine having to write the extension adding any value here.

lrhn commented 2 years ago

@tatumizer Dart uses paths (in URIs) do designate libraries. This avoids having to introduce a namespace that "canonical" names are resolved against. (That wasn't always the goal, hence the library name; declaration, but it was never clear what the namespace would be). Having the compiler need to know all libraries in the current Pub package and all available other packages before it can resolve an import is not practical or efficient.

So, analyzer:dart/ast/visitor uniquely points to the analyzer package, which the compiler knows the location of (through the package configuration that you have to do before compiling). It then treats dart/ast/visitor as something related to that package location. Again, we use it as a path because then we don't need to know every library in order to find dart/ast/visitor (and we can canonicalize URIs, because as an URI, dart/ast/../ast/visitor is the same thing, and canonicalization only works because we know that it's a hierarchical path).

This is a proposal to introduce a shorthand for URI-based imports. It's not introducing new ways to name files, at the end, the result is going to be a URI anyway.

So, saying, it is a path. There is no attempt to make it anything else, or to hide that it is a path. Any non-path-like syntax will eventually have to be resolved back to an actual path. People reading analyzer:dart/ast/visitor are expected to understand, at some level, that they are importing "package:analyzer/dart/ast/visitor.dart". You need to know that, because if test/foo_test.dart needs to refer to lib/src/test_helper.dart, you need to know how to map that to my_package:src/test_helper.

leafpetersen commented 2 years ago

I discussed this with @lrhn this morning, and I think made some progress towards alleviating my concerns. Much of the confusion for me is rooted in the fact that both the new syntax and the old syntax use :, but for completely unrelated things. Since my immediate response was to try to match up the : in the two formats, the result seemed very ad hoc and confusing. The correct intuition, I believe is the following:

Overall, this model makes sense to me, and I think is explainable.

The choice to use : as the separator token, is, I think going to be a source of confusion since it suggests a false connection to the full syntax, but it may be the only reasonable choice (are there other better choices?).

Neither of us are (so far) convinced by the arguments in favor of a dotted notation. Using / as the path separator is familiar to existing Dart programmers, and I think unlikely to be confusing to new Dart programmers.

I still don't love that there are now multiple ways to write the same import, but with guidance and linting towards using a canonical style except when necessary for escaping, I think this is ok.