Fix use insertion when do block starts with destructuring bind
#5911
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pchiusano
commented
Oct 2, 2025
Comment on lines
1358
to
1371
| reannotateUp g t = case ABT.out t of | ||
| ABT.Var v -> ABT.annotatedVar (annotation t, g t) v | ||
| ABT.Cycle body -> | ||
| let body' = reannotateUp g body | ||
| in ABT.cycle' (annotation t, snd (annotation body')) body' | ||
| ABT.Abs v body -> | ||
| let body' = reannotateUp g body | ||
| in ABT.abs' (annotation t, snd (annotation body')) v body' | ||
| ABT.Tm body -> | ||
| -- literals like 0xsaaff don't contribute to the annotations | ||
| -- even though they desugar to function calls | ||
| let body' = reannotateUp g <$> body | ||
| ann = if isLiteral t then mempty else g t <> foldMap (snd . annotation) body' | ||
| in ABT.tm' (annotation t, ann) body' |
Member
Author
There was a problem hiding this comment.
This function didn't quite exist in ABT and it's too special purpose to add there since it's specific to the Unison term functor - nodes that are bytes literals have their annotation zero'd out to avoid contributing to use clauses that are inserted higher in the syntax tree.
Comment on lines
-1889
to
+1913
| pattern Bytes' :: [Word64] -> Term3 v PrintAnnotation | ||
| pattern Bytes' :: [Word64] -> Term2 v at ap v a | ||
| pattern Bytes' bs <- (toBytes -> Just bs) | ||
|
|
||
| toBytes :: Term3 v PrintAnnotation -> Maybe [Word64] | ||
| toBytes :: Term2 v at ap v a -> Maybe [Word64] |
Member
Author
There was a problem hiding this comment.
Needed to generalize these signatures to get it to typecheck. They were needlessly monomorphic.
Comment on lines
-320
to
+341
| Delay' x | ||
| | Match' _ _ <- x -> do | ||
| Delay' x@(Match' scrutinee cs) | ||
| | not (isDestructuringBind scrutinee cs) -> do | ||
| px <- pretty0 (ac Annotation Block im doc) x | ||
| let hang = if isSoftHangable x then PP.softHang else PP.hang | ||
| pure . paren (p > Control) $ | ||
| fmt S.ControlKeyword "do" `hang` px | ||
| | otherwise -> do | ||
| let (im0', uses0) = calcImports im x | ||
| let allowUses = isLet x || (p == Bottom) | ||
| let im' = if allowUses then im0' else im | ||
| let uses = if allowUses then uses0 else [] | ||
| let soft = isSoftHangable x && null uses && p < Annotation | ||
| let hang = if soft then PP.softHang else PP.hang | ||
| px <- pretty0 (ac Annotation Block im' doc) x | ||
| -- this makes sure we get proper indentation if `px` spills onto | ||
| -- multiple lines, since `do` introduces layout block | ||
| let indent = PP.Width (if soft then 2 else 0) + (if soft && p < Application then 1 else 0) | ||
| pure . paren (p > Control) $ | ||
| fmt S.ControlKeyword "do" `hang` PP.lines (uses <> [PP.indentNAfterNewline indent px]) | ||
| Delay' x -> do | ||
| let (im0', uses0) = calcImports im x | ||
| let allowUses = isLet x || (p == Bottom) || isDestructure x | ||
| where | ||
| isDestructure (Match' scrutinee cs) = isDestructuringBind scrutinee cs | ||
| isDestructure _ = False | ||
| let im' = if allowUses then im0' else im | ||
| let uses = if allowUses then uses0 else [] | ||
| let soft = isSoftHangable x && null uses && p < Annotation | ||
| let hang = if soft then PP.softHang else PP.hang | ||
| px <- pretty0 (ac Annotation Block im' doc) x | ||
| -- this makes sure we get proper indentation if `px` spills onto | ||
| -- multiple lines, since `do` introduces layout block | ||
| let indent = PP.Width (if soft then 2 else 0) + (if soft && p < Application then 1 else 0) | ||
| pure . paren (p > Control) $ | ||
| fmt S.ControlKeyword "do" `hang` PP.lines (uses <> [PP.indentNAfterNewline indent px]) |
Member
Author
There was a problem hiding this comment.
This logic is finnicky but it is the same as before, except that we allow use insertion if the first thing in a do is a destructuring bind.
runarorama
approved these changes
Oct 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the pretty-printer would fail to insert a
useclause at the start of adoblock, if thatdoblock began with a destructuring bind. This could sometimes lead to some terrible rendering with chains of operators all showing up with qualified names.Here's a minimal example. There are two things called
++, azoink.++andfoo.++. Thus, the code must disambiguate with auseclause:The
useclause wasn't being used when viewingexamplebecause thedoblock starts with aMatch, which isn't typically considered a candidate spot foruseinsertion. But actually, if thematchhappens to be a destructuring bind, putting auseclause there is fine.Here's before and after -
Before:
After:
Testing
I added a transcript test and it passes all the existing transcript tests.
Misc
I fixed what ended up being an unrelated bug with
Bytesliterals causing imports ofBytes.fromList.impl.