directives_ordering unexpectedly changed

[yangsfang]

Describe the issue
The linter for the following seems to have broken by #4538

import '/foo1.dart';
import '../foo2.dart';
import 'foo3.dart';

is being flagged, and changed to

import '../foo2.dart';
import '/foo1.dart';
import 'foo3.dart';

To Reproduce
Create a dart file with the above import. Run dart lint <file>.

Expected behavior
Expected /foo1.dart to be at the top as it is an absolute path. '../' and './' are all relative paths. This was the behavior before the PR was merged.

import '/foo1.dart';
import '../foo2.dart';
import 'foo3.dart';

Additional context
#4538 was fixing #2678 which is unrelated to the issue above. The example given in #4538 had only relative paths. The proposed fix should not have affected absolute path ordering.

import './foo1.dart';
import '../../foo2.dart';
import '../foo3.dart';
import 'foo4.dart';

[srawlins]

CC @oprypin

[lrhn]

Should we use some kind of logical ordering instead of accidental lexicographical ordering?
Fx. order by distance to current directory, and even normalizing the URIs to avoid any . path segments or non-leading .. segments?

The example

import './foo1.dart';
import '../../foo2.dart';
import '../foo3.dart';
import 'foo4.dart';

shows './foo1.dart' which is equivalent to 'foo1.dart'. We should normalize that so it's sorted next to 'foo4.dart'.

If there are no leading or embedded .s, then the rest are actually ordered nicely because ../../ goes before ../, which means the directories com in "closer to root first" order (and with rooted paths going before relative paths, that feels right).

Generally, it should sort by ordering by path segment, rather than treating / as just a character (if it doesn't already, haven't checked. 😊)

[bwilkerson]

We are currently enforcing the guidelines in the style guide (https://dart.dev/effective-dart/style#ordering). If we want to update the style guide then we'll update the lint (and sort command) to correspond. Assuming that it has the right definition of "alphabetical", I believe that this is currently working as intended (modulo the bug that was introduced).

@munificent For visibility.

@srawlins Did you recently review a CL with a fix for this bug?

[srawlins]

Yes, "Fixes XYZ" does not work cross-repos, so every linter bug must be closed manually. ☹️

Fixed by dart-lang/sdk@9fdb0ca

[yangsfang]

We are currently enforcing the guidelines in the style guide (https://dart.dev/effective-dart/style#ordering). If we want to update the style guide then we'll update the lint (and sort command) to correspond. Assuming that it has the right definition of "alphabetical", I believe that this is currently working as intended (modulo the bug that was introduced).

@munificent For visibility.

@srawlins Did you recently review a CL with a fix for this bug?

Well. I do not believe this reason is proper for closing the issue. Here is a quote from Effective dart that was linked:

DO place dart: imports before other imports

This issue does not deal with that. We are dealing with "other imports" exclusively.

DO place package: imports before relative imports

This issue does not deal with that. We are dealing with "relative imports" (relative to its own package) exclusively.

DO specify exports in a separate section after all imports

This issue does not deal with exports.

DO sort sections alphabetically

Emphasis added. Slashes are not included in the (English) alphabet. The example also implies that the author meant the English alphabet.

Effective dart did not specify whether the ordering should be strictly lexicographical according to the ASCII alphabet, which would be honestly quite arbitrary.

At the very least, please update effective Dart to document such ordering as intended. (e.g. change "alphabetical" to "lexicographical")

---

Should we use some kind of logical ordering instead of accidental lexicographical ordering? Fx. order by distance to current directory, and even normalizing the URIs to avoid any . path segments or non-leading .. segments?

I would definitely support this idea. Google's style guide for C/C++ does have a list of ordering it from farthest to closest to the current file: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

[yangsfang]

Never mind, silly me. I thought the issue was closed as "WAI", but it is in fact fixed. Please ignore my previous comment.

Thanks for the fix.

[munificent]

Should we use some kind of logical ordering instead of accidental lexicographical ordering?

Eh, probably not worth the effort. Lexicographical is close enough to logical order except in the case where users are pointlessly doing ./ which they can just... not do. We aren't Node and you don't need Node-style import paths in Dart. :)