dartfmt on enormous expression is slow and leaves a half-formatted long lines #456
Labels
Comments
munificent
added a commit
that referenced
this issue
Nov 10, 2015
The constraint checking was quadratic in the number of rules in the line which is bad on pathologically long lines like #456. This gets it back down to linear. It takes that bug from 7 minutes to about 20 seconds on my machine. The benchmark, which is closer to real code, gets about 20% faster. I'm still leaving #456 open because I think there's more work to be done there, but this is a solid improvement overall. [email protected] Review URL: https://codereview.chromium.org//1418483008 .
|
Might be a related issue: when I run dartfmt on a file with some long and deep expressions, it "unformats" the file with lines longer than 80 chars, even for lines I have manually split. |
|
Yes, if you have particularly huge expressions—which are generally hard to maintain anyway—in rare cases the formatter can't find a solution and eventually gives up. If you have an example you're willing to share, feel free to add it in a comment here and I'll use it as a regression test if I get a chance to fix this. |
|
I have another reproduction case that doesn't seem too crazy: new LibraryBuilder()
.addImport((i) => i.setUri('coffee_app.dart'))
.addClass((c) => c
.setName('Coffee\$Injector')
.setImplements(['Coffee'])
.addField((f) => f
.setFinal()
.setType((t) => t.setIdentifier('DripCoffeeModule'))
.setName('_dripCoffeeModule'))
.addMethod((m) => m
.setName('Coffee\$Injector')
.addParameter((p) => p.setThis('_dripCoffeeModule')))
.addMethod((m) => m
.addAnnotation((a) => a.override())
.setReturnType((t) => t.setIdentifier('CoffeeMaker'))
.setName('getCoffeeMaker')
.setBodyExpression(
// NOTE: Below this point, dartfmt will give up.
(b) => b
.setExpression((e) => e
.invoke()
.setMethod('new CoffeeMaker')
.addArgument((x) => x
.invoke()
.setMethod('_dripCoffeeModule.provideHeater'))
.addArgument((x) => x
.invoke()
.setMethod('_dripCoffeeModule.providePump'))))));After running dartfmt, it results in (just the commented part from above): (b) => b
.setExpression((e) => e.invoke().setMethod('new CoffeeMaker').addArgument((x) => x.invoke().setMethod('_dripCoffeeModule.provideHeater')).addArgument((x) => x.invoke().setMethod('_dripCoffeeModule.providePump'))))));Internally this is change 130805683. |
Says their repro isn't too crazy. Follows it with a 774 character single statement containing expression-bodied lambdas nested five levels deep. 😜 I agree dartfmt should handle this callback-based builder idiom better because it's pretty common. But it's also quite a beast if you just look at it through the lens of "this is a regular statement of general Dart code". |
|
Doesn't seem too crazy to me ;) Not a P1, but would be nice to support ❤️ |
|
Here's another one: class Aaa {
@AaaaAaaaaaaaaa()
static AaaaaAaaaAaaaa aaaaaaaaaaa(AaaaaAaaaAaaaa aaaaa, AaaaaAaaaAaaaa aaaaa,
aaaaaa aaaaaaaAaaaaa) =>
new AaaaaAaaaAaaaa()
..aaaaAaaaaa = new Aaa64((aaaaa.aaaaAaaaaa.aaAaaaaa() +
((aaaaa.aaaaAaaaaa - aaaaa.aaaaAaaaaa).aaAaaaaa() *
aaaaaaaAaaaaa))
.aaAaa())
..aaaAaaAaaaaa = new Aaa64((aaaaa.aaaAaaAaaaaa.aaAaaaaa() +
((aaaaa.aaaAaaAaaaaa - aaaaa.aaaAaaAaaaaa).aaAaaaaa() *
aaaaaaaAaaaaa))
.aaAaa())
..aaaaaa =
aaaaa.aaaaaa + ((aaaaa.aaaaaa - aaaaa.aaaaaa) * aaaaaaaAaaaaa)
..aaaaaaaaaaa = aaaaa.aaaaaaaaaaa +
((aaaaa.aaaaaaaaaaa - aaaaa.aaaaaaaaaaa) * aaaaaaaAaaaaa)
..aaaaAaaaa = aaaaa.aaaaAaaaa +
((aaaaa.aaaaAaaaa - aaaaa.aaaaAaaaa) * aaaaaaaAaaaaa)
..aaaaaaaaAaaaaaaaaaa = aaaaa.aaaaaaaaAaaaaaaaaaa +
((aaaaa.aaaaaaaaAaaaaaaaaaa - aaaaa.aaaaaaaaAaaaaaaaaaa) *
aaaaaaaAaaaaa)
..aaaaa =
aaaaa.aaaaa + ((aaaaa.aaaaa - aaaaa.aaaaa) * aaaaaaaAaaaaa)
..aaaAaaaaaaaaaa = aaaaa.aaaAaaaaaaaaaa
..aaaaAaaaaaaa = aaaaa.aaaaAaaaaaaa;
} |
|
Here is another example from the SDK tests standalone_2/io/file_system_async_links_test.dart: return new Link(y)
.create(x)
.then((link) => Expect.equals(y, link.path))
.then((_) => FutureExpect.isFalse(new File(y).exists()))
.then((_) => FutureExpect.isFalse(new File(x).exists()))
.then((_) => FutureExpect.isTrue(FileSystemEntity.isLink(y)))
.then((_) => FutureExpect.isFalse(FileSystemEntity.isLink(x)))
.then((_) => FutureExpect.equals(
FileSystemEntityType.notFound, FileSystemEntity.type(y)))
.then((_) => FutureExpect.equals(
FileSystemEntityType.notFound, FileSystemEntity.type(x)))
.then((_) => FutureExpect.equals(FileSystemEntityType.link,
FileSystemEntity.type(y, followLinks: false)))
.then((_) => FutureExpect.equals(FileSystemEntityType.notFound,
FileSystemEntity.type(x, followLinks: false)))
.then((_) => FutureExpect.equals(x, new Link(y).target()))
.then((_) => new File(y).create())
.then((yFile) => Expect.equals(y, yFile.path))
.then((_) => FutureExpect.isTrue(new File(y).exists()))
.then((_) => FutureExpect.isTrue(new File(x).exists()))
.then((_) => FutureExpect.isTrue(FileSystemEntity.isLink(y)))
.then((_) => FutureExpect.isFalse(FileSystemEntity.isLink(x)))
.then((_) => FutureExpect.isTrue(FileSystemEntity.isFile(y)))
.then((_) => FutureExpect.isTrue(FileSystemEntity.isFile(x)))
.then((_) => FutureExpect.equals(
FileSystemEntityType.file, FileSystemEntity.type(y)))
.then((_) => FutureExpect.equals(
FileSystemEntityType.file, FileSystemEntity.type(x)))
.then((_) => FutureExpect.equals(FileSystemEntityType.link,
FileSystemEntity.type(y, followLinks: false)))
.then((_) => FutureExpect.equals(FileSystemEntityType.file,
FileSystemEntity.type(x, followLinks: false)))
.then((_) => FutureExpect.equals(x, new Link(y).target()))
.then((_) => new File(x).delete())
.then((xDeletedFile) => Expect.equals(x, xDeletedFile.path))
.then((_) => new Directory(x).create())
.then((xCreatedDirectory) => Expect.equals(x, xCreatedDirectory.path))
.then((_) => FutureExpect.isTrue(FileSystemEntity.isLink(y)))
.then((_) => FutureExpect.isFalse(FileSystemEntity.isLink(x)))
.then((_) => FutureExpect.isTrue(FileSystemEntity.isDirectory(y)))
.then((_) => FutureExpect.isTrue(FileSystemEntity.isDirectory(x)))
.then((_) => FutureExpect.equals(
FileSystemEntityType.directory, FileSystemEntity.type(y)))
.then((_) => FutureExpect.equals(
FileSystemEntityType.directory, FileSystemEntity.type(x)))
.then((_) => FutureExpect.equals(FileSystemEntityType.link,
FileSystemEntity.type(y, followLinks: false)))
.then((_) => FutureExpect.equals(FileSystemEntityType.directory,
FileSystemEntity.type(x, followLinks: false)))
.then((_) => FutureExpect.equals(x, new Link(y).target()))
.then((_) => new Link(y).delete())
.then((_) => FutureExpect.isFalse(FileSystemEntity.isLink(y)))
.then((_) => FutureExpect.isFalse(FileSystemEntity.isLink(x)))
.then((_) => FutureExpect.equals(
FileSystemEntityType.notFound, FileSystemEntity.type(y)))
.then(
(_) => FutureExpect.equals(FileSystemEntityType.directory, FileSystemEntity.type(x)))
.then((_) => FutureExpect.throws(new Link(y).target()))
.then((_) => temp.delete(recursive: true)); |
|
These all format nearly instantaneously using the forthcoming tall style. \o/ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Formatting took over 7 minutes
The original expression was reasonably formatted and each line was <80 columns.
The formatter gave up about 1/3 the way through and generated very long lines.
The text was updated successfully, but these errors were encountered: