Redesign ExprChain around a head, segments and a terminal call - #3385
Open
nojaf wants to merge 11 commits into
Open
Redesign ExprChain around a head, segments and a terminal call#3385nojaf wants to merge 11 commits into
nojaf wants to merge 11 commits into
Conversation
The old model held a chain as a flat `ChainLink list` with an `isLastLink` flag deciding whether a space could precede a call's opening paren. Position and permission to add a space are only incidentally the same thing, and they came apart inside a dot-lambda body: with `space_before_uppercase_invocation` enabled, `_.Substring(0, 16).ToLower()` gained a space and stopped compiling, because the printer had no way to know it was inside a `_.` lambda (fsprojects#3364). A chain is now a head, a list of `ChainSegment`, and a `ChainTerminal`. Each segment carries its own dot, since a dot always belongs with what follows it, which makes two adjacent dots unrepresentable. Only the terminal negotiates a space. That is a grammar constraint rather than a style choice: a space mid-chain reparses `a.Foo (x).Bar()` as `a.Foo ((x).Bar())`. `ChainTerminal.NoSpaceAllowed` therefore makes 'no space is permitted here' a property of the node, instead of something the printer has to rediscover from context. Nodes that were chains in all but name are absorbed, leaving `Expr.Chain` as the single representation of a dotted expression: - ExprDotLambda - ExprAppLongIdentAndSingleParenArg - ExprDotIndexedGet - AppWithLambda with no prefix arguments ExprNestedIndexWithoutDot is removed as dead; nothing ever constructed it. This breaks the public Oak API: a dotted long ident such as `a.b.c` now yields `Expr.Chain` rather than `Expr.OptVar` in expression position. Layout changes follow from the model. A chain whose only call is its last step keeps the receiver, navigation and method name together and breaks the call's arguments, exactly as a call with no receiver would. Two or more calls, or a call followed by further navigation, use the leading-dot pipeline. A chain of pure property access fills lines greedily rather than fanning out one member per line. docs/docs/contributors/Chains.md states the rules in full; they are intended for the F# style guide and live under Contributors until they are adopted there. Fixes a second source of invalid F#, unrelated to spacing. A conditional directive attached to an intermediate call's argument pushed the opening paren onto its own line, which reparses the chain (FCS error 597). The paren now stays welded to the member name and the break lands after it. Branches in the chain transformer that are unreachable by construction now raise the new InvariantViolationException rather than silently returning an empty list. It derives from FormatException, which matters because the CLI selects its message by exception type and a bare failwith fell through to an empty message at normal verbosity. Each carries the source range of the construct involved and points at fantomas-tools. Adds a Coverage pipeline (dotnet fsi build.fsx -- -p Coverage) built on AltCover and ReportGenerator. It is how the unreachable branches were identified, and how two branches that looked dead were shown to be reachable but untested, which would have turned valid F# into a crash. global.json rolls forward to the latest SDK feature band so the repo builds with only 10.0.301 installed.
A run of navigation steps that does not fit on one line used to be filled
greedily, leaving one line packed to the margin and a short stub behind it.
Such a run is now wrapped so that the longest resulting line is as short as
possible; when two wraps tie, the longer first line wins.
Two refinements came out of running this over real code. The starting value
keeps a step beside it rather than sitting alone, since a line holding only
the starting value has nothing on it to balance. And moving navigation down
is preferred over breaking a call's arguments, so a call stays whole
whenever some wrap can hold it. The latter also fixes a navigation run in
the middle of a pipeline never wrapping at all, which used to force the
following call's arguments open instead.
Match lambdas no longer depend on where their call sits in the chain.
`(function` is now laid out like `(fun`, which is what the F# style guide
asks for ("Treat match lambda's in a similar fashion"). The position
dependent behaviour was inherited from issue 1804 and had no counterpart
for `fun`.
Chains.md is restructured around the five steps the printer takes, and now
shows the alternatives that were rejected next to what Fantomas produces,
so the reasoning behind each rule is visible rather than implied.
Output at the default max line length is unchanged: formatting this
repository's own sources at widths 120 and 80 gives no difference, and only
four files differ at 60.
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
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.
The old model held a chain as a flat
ChainLink listwith anisLastLinkflag deciding whether a space could precede a call's opening paren. Position and permission to add a space are only incidentally the same thing, and they came apart inside a dot-lambda body: withspace_before_uppercase_invocationenabled,_.Substring(0, 16).ToLower()gained a space and stopped compiling, because the printer had no way to know it was inside a_.lambda (#3364).A chain is now a head, a list of
ChainSegment, and aChainTerminal. Each segment carries its own dot, since a dot always belongs with what follows it, which makes two adjacent dots unrepresentable. Only the terminal negotiates a space. That is a grammar constraint rather than a style choice: a space mid-chain reparsesa.Foo (x).Bar()asa.Foo ((x).Bar()).ChainTerminal.NoSpaceAllowedtherefore makes 'no space is permitted here' a property of the node, instead of something the printer has to rediscover from context.Nodes that were chains in all but name are absorbed, leaving
Expr.Chainas the single representation of a dotted expression:ExprNestedIndexWithoutDot is removed as dead; nothing ever constructed it. This breaks the public Oak API: a dotted long ident such as
a.b.cnow yieldsExpr.Chainrather thanExpr.OptVarin expression position.Layout changes follow from the model. A chain whose only call is its last step keeps the receiver, navigation and method name together and breaks the call's arguments, exactly as a call with no receiver would. Two or more calls, or a call followed by further navigation, use the leading-dot pipeline. A chain of pure property access fills lines greedily rather than fanning out one member per line. docs/docs/contributors/Chains.md states the rules in full; they are intended for the F# style guide and live under Contributors until they are adopted there.
Fixes a second source of invalid F#, unrelated to spacing. A conditional directive attached to an intermediate call's argument pushed the opening paren onto its own line, which reparses the chain (FCS error 597). The paren now stays welded to the member name and the break lands after it.
Branches in the chain transformer that are unreachable by construction now raise the new InvariantViolationException rather than silently returning an empty list. It derives from FormatException, which matters because the CLI selects its message by exception type and a bare failwith fell through to an empty message at normal verbosity. Each carries the source range of the construct involved and points at fantomas-tools.
Adds a Coverage pipeline (dotnet fsi build.fsx -- -p Coverage) built on AltCover and ReportGenerator. It is how the unreachable branches were identified, and how two branches that looked dead were shown to be reachable but untested, which would have turned valid F# into a crash.