Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adjust heuristic for fun-decl body placement #88

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Adjust heuristic for fun-decl body placement #88

wants to merge 2 commits into from

Conversation

MatthewFluet
Copy link

Don't force "biggish" fun-decl-body expressions onto new line.

Fixes #82

While this addresses the main issue raised by #82, it does not exactly replicate the logic of val decls. I tried doing something closer to PrettierExpAndDec.sml#L815:L817, with a showClauseSplittable that uses cond and splitShowExpLeft/splitShowExpRight, but I found the isSplittable condition for App to frequently leave a single token on the fun line after the = token, followed by a much larger expression on the next line, which often looked awkward if not outright misleading; for example:

-    fun indentedAtLeastBy i =                                                                                                                                    
-      S { indent = Indented {minIndent = SOME i, maxIndent = NONE}                                                                                               
-        , rigid = false                                                                                                                                          
-        , allowsComments = false                                                                                                                                 
-        }                                                                                                                                                        
+    fun indentedAtLeastBy i = S                                                                                                                                  
+      { indent = Indented {minIndent = SOME i, maxIndent = NONE}                                                                                                 
+      , rigid = false                                                                                                                                            
+      , allowsComments = false                                                                                                                                   
+      }

or

-          fun continue i =                                                                                                                                       
-            not (isReserved Token.Colon i orelse isReserved Token.Equal i)                                                                                       
+          fun continue i = not                                                                                                                                   
+            (isReserved Token.Colon i orelse isReserved Token.Equal i)

@shwestrick
Copy link
Owner

One downside of removing the isBiggish check is that it can cause deep rightward drift, e.g. the following. (The common pattern of fun with a let in end body was my original motivation for adding isBiggish.) Personally, I'd really like to avoid the rightward drift, especially for let in end.

-  fun new () =
-    let val result = DocVar {id = !counter}
-    in counter := !counter + 1; result
-    end
+  fun new () = let val result = DocVar {id = !counter}
+               in counter := !counter + 1; result
+               end

Of course, as you point out in #82, this is inconsistent with how val bindings are handled.
And, val bindings suffer from the awkward splitting example above...

This makes me think that we need a heuristic to avoid awkward splitting. And, if we had this heuristic, then the same splitting logic could be used for both vals and funs. I.e., both would use the isBiggish heuristic, and both would also have an additional check to avoid awkward splitting.

Thoughts?

@MatthewFluet
Copy link
Author

The issue with rightward drift makes sense. Agreed that a heuristic that avoids the awkward splitting and is consistent with both val and fun would be best.

I think that the issue with isBiggish is that it is a property of the expression's abstract syntax, not a property of the way that the expression is printed. In particular, the fun examples in #82 are "biggish" because they trip the depth >= 3 condition due to applications. Yet, they have small printing width and can easily fit on the same line as the fun.

The informal heuristic would seem to be that if the entire expression can be printed on the same line as the fun/val's =, then do so; else drop the expression to the next line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Heuristic for placing fun decl body on separate line
2 participants
@MatthewFluet @shwestrick