Anthony Bullard (Jan 10 2025 at 15:32): Anthony Bullard (Jan 10 2025 at 15:32):I have one snapshot test failing on re-formatting and I honestly just don't understand the progression. I've updated the output of the case to be helpful, but here are the original/formatted/reformatted asts (with the source code below). I've lined up the relevant nodes with each other
Screenshot 2025-01-10 at 9.28.32 AM.png
For those like @Joshua Warner or @Sam Mohr who are familiar with this where do you think this SpaceAfter comes from around the PncApply in the reformatted AST?
Sam Mohr (Jan 10 2025 at 15:34):Can you paste the source code?
Sam Mohr (Jan 10 2025 at 15:34):Oh, I see it
Sam Mohr (Jan 10 2025 at 15:34):In the bottom of the image
Anthony Bullard (Jan 10 2025 at 15:35):Yeah so the extra newline coming in is what is perplexing, and what makes the formatting so terrible
Sam Mohr (Jan 10 2025 at 15:35):Yep, the formatting that comes as "step 2" here makes sense to me
Sam Mohr (Jan 10 2025 at 15:36):The re-formatting is weird indeed
Sam Mohr (Jan 10 2025 at 15:36):But I think it comes from how we format patterns
Anthony Bullard (Jan 10 2025 at 15:36):But I think it's the parsing of the formatted AST
Anthony Bullard (Jan 10 2025 at 15:37):Or I guess not
Sam Mohr (Jan 10 2025 at 15:37):If you look at the docs for Str.walkUtf8, whenever we have a multiline function type, we make sure the arrow is indented
Anthony Bullard (Jan 10 2025 at 15:37):It would be the formatted of the formatted ast
Sam Mohr (Jan 10 2025 at 15:37):Which makes sense for types
Sam Mohr (Jan 10 2025 at 15:37):But this is a value, right?
Anthony Bullard (Jan 10 2025 at 15:37):This is a value, you can see the top level node is an Expr
Sam Mohr (Jan 10 2025 at 15:38):So either our formatting for PNC patterns has some artifacts from formatting multiline spaced patterns
Anthony Bullard (Jan 10 2025 at 15:38):So it's an Expr::Closure, and the first list is a [Pattern<'_>]
Sam Mohr (Jan 10 2025 at 15:39):I'll look at the code in a few minutes
Anthony Bullard (Jan 10 2025 at 15:39):It's on my branch, maybe I did something goofy aligning it with Expr formatting
Sam Mohr (Jan 10 2025 at 15:39):Trying to finish up this invalid ident warning code
Anthony Bullard (Jan 10 2025 at 15:40):No worries. I have to go to work and write and test some Java.
Anthony Bullard (Jan 10 2025 at 15:40):Shed a tear for me
Joshua Warner (Jan 10 2025 at 16:13):Historically this sort of thing happened when is_multiline or one of the matches in the outer scopes differed slightly in details from how the child is actually formatted (e.g. adding/removing parens).
Now, these bugs have generally been pushed into <>.to_node(), expr_lift_spaces, pattern_lift_spaces, etc. in such a way that those functions are not sufficiently "normalizing".
In the initial formatted version (the middle column of your screenshot), it looks to me like the closure is for some reason incorrectly thinking that the arg list is _not_ multiline - and then it turning out it actually is when formatted.
Anthony Bullard (Jan 10 2025 at 20:49):I think this is the issue:
buf.push('\\');
let arguments_are_multiline = loc_patterns
.iter()
.any(|loc_pattern| loc_pattern.is_multiline());
// If the arguments are multiline, go down a line and indent.
let indent = if arguments_are_multiline {
indent + INDENT
} else {
indent
};
let mut first = true;
for loc_pattern in loc_patterns.iter() {
if !first {
buf.indent(indent);
if arguments_are_multiline {
buf.push(',');
buf.newline();
} else {
buf.push_str(",");
buf.spaces(1);
}
}
first = false;
let arg = pattern_lift_spaces(buf.text.bump(), &loc_pattern.value);
if !arg.before.is_empty() {
fmt_comments_only(buf, arg.before.iter(), NewlineAt::Bottom, indent)
}
arg.item
.format_with_options(buf, Parens::InClosurePattern, Newlines::No, indent);
if !arg.after.is_empty() {
if starts_with_inline_comment(arg.after.iter()) {
buf.spaces(1);
}
fmt_comments_only(buf, arg.after.iter(), NewlineAt::Bottom, indent)
}
}
The comment doesn't tell the truth
Anthony Bullard (Jan 10 2025 at 20:50):Nor is it correct - in my opinion
Anthony Bullard (Jan 10 2025 at 20:50):We do not go down a line and indent. We just increase the indent
Anthony Bullard (Jan 10 2025 at 20:51):And I'm not sure we should do that at all for a single multi-line pattern
Anthony Bullard (Jan 10 2025 at 20:53):I think I'm going to say "If arguments_are_multiline AND there are more than 1 argument - actually go down a line and indent with an increased indent. otherwise keep the same indent and have the arrow be on the same line as the end of the arg pattern.
Anthony Bullard (Jan 10 2025 at 20:53):Does this sound correct?
Sam Mohr (Jan 10 2025 at 20:53):That sounds correct, but I'd have to see examples get formatted in the PR
Anthony Bullard (Jan 10 2025 at 20:55):So:
\
SomeThing({
with: 'multilining',
more: 'args',
}),
foo
-> ....
But:
\L(
z, # This is multiline
) -> ....
That works for now. I don't like the \ being on a different line, but we're killing it soon, so :shrug:
Anthony Bullard (Jan 10 2025 at 20:56):But maybe we also want:
\foo, L(
z, # This is multiline
) -> ....
Yeah
Anthony Bullard (Jan 10 2025 at 20:56):|
SomeThing({
with: 'multilining',
more: 'args',
}),
foo
| ....
looks better I guess?
Sam Mohr (Jan 10 2025 at 20:57):Not great, but better
Sam Mohr (Jan 10 2025 at 20:57):I don't think we can do better than that, though, so it's good
Anthony Bullard (Jan 10 2025 at 20:57):So I think the rule is if there is a multiline arg _before the last arg_, then we do the whole outdenting thing
Anthony Bullard (Jan 10 2025 at 20:59):Hmmm...thinking. That would mean with || syntax:
|foo, SomeThing(
z, # This is multiline
) | ....
Is that better than:
|
foo,
SomeThing(
z, # This is multiline
),
| ....
?
Sam Mohr (Jan 10 2025 at 21:00):I like symmetry, so I vote 2
Sam Mohr (Jan 10 2025 at 21:00):Maybe that's just me
Sam Mohr (Jan 10 2025 at 21:01):Function args have different aesthetic rules than function bodies
Joshua Warner (Jan 10 2025 at 21:01):I agree that comment doesn't match the code, and that the current formatting is not ideal - but I'm not sure that's the cause of the bug you're looking at
Anthony Bullard (Jan 10 2025 at 21:02):Ok, So for now I'll do if more than one arg do the outdent. But with || I'll go back to doing it for any number of args that are multiline
Anthony Bullard (Jan 10 2025 at 21:02):Joshua Warner said:
I agree that comment doesn't match the code, and that the current formatting is not ideal - but I'm not sure that's the cause of the bug you're looking at
We'll find out soon
Anthony Bullard (Jan 10 2025 at 21:02):But I have some other things to do first
Last updated: Jul 06 2025 at 12:14 UTC