smlfmt icon indicating copy to clipboard operation
smlfmt copied to clipboard

Adjust heuristic for fun-decl body placement

Open MatthewFluet opened this issue 1 year ago • 2 comments

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)                                                                                           

MatthewFluet avatar May 17 '23 00:05 MatthewFluet

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?

shwestrick avatar May 26 '23 14:05 shwestrick

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.

MatthewFluet avatar May 26 '23 17:05 MatthewFluet