Skip to content

Commit d67eb97

Browse files
nojafnojaf
committed
Guard the tool against printf and test it under Native AOT in CI
printf specializes its formatters at runtime, which a Native AOT build cannot do, and whether a given format survives depends on what the AOT compiler happened to generate. The previous commits took printf off the paths the tool runs; this keeps it off and checks the result. FANTOMAS-PRINTF-001 reports, at error severity, every function that takes a printf format and every interpolated string with a hole the compiler does not lower to String.Concat, in src/Fantomas and src/Fantomas.Core. It reads the untyped tree and states the lowering of dotnet/fsharp#19971 itself, because the typed tree comes from the analyzer SDK's own compiler, which predates it. A test in Fantomas.Tests checks the same against the compiled assemblies, by looking for PrintfFormat constructions in their IL. The uses that remain are ones the tool never reaches, and each is suppressed or allowed with the reason beside it. Two fallbacks and a DEBUG-only failwithf are rewritten without printf instead. The TestAot pipeline publishes the tool with Native AOT for the machine it runs on and runs Fantomas.Tests against that build. A new CI job runs it on Linux, Windows and macOS. When FantomasAot is set, the lock files go under artifacts/obj, so a publish for a runtime identifier no longer rewrites the checked in ones. The vendored compiler now comes from the dotnet/fsharp#20658 branch on a fork, which builds parser diagnostics without printf or runtime reflection. FCSRepository in Directory.Build.props names the repository to download from, read through MSBuild, and goes back to dotnet/fsharp once that pull request is merged.
1 parent aec6e13 commit d67eb97

19 files changed

Lines changed: 821 additions & 110 deletions

‎.github/workflows/main.yml‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,27 @@ jobs:
3737
github_token: ${{ secrets.GITHUB_TOKEN }}
3838
publish_dir: ./output
3939

40+
# Publishes the tool with Native AOT and runs its tests against that build. AOT compiles for the
41+
# machine it runs on, which is why every operating system gets a run of its own.
42+
aot:
43+
name: Native AOT
44+
strategy:
45+
fail-fast: false
46+
matrix:
47+
os: [ubuntu-latest, windows-latest, macOS-latest]
48+
runs-on: ${{ matrix.os }}
49+
steps:
50+
- uses: actions/checkout@v7.0.1
51+
- name: Setup .NET Core
52+
uses: actions/setup-dotnet@v6
53+
with:
54+
cache: true
55+
cache-dependency-path: Directory.Packages.props
56+
- name: Init
57+
run: dotnet fsi build.fsx -p Init
58+
- name: Test against Native AOT
59+
run: dotnet fsi build.fsx -p TestAot
60+
4061
analyze:
4162
name: Analyze solution
4263
runs-on: ubuntu-latest

‎Directory.Build.props‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,26 @@ Some common use cases include:
4343
<OtherFlags>$(OtherFlags) --realsig+</OtherFlags>
4444
</PropertyGroup>
4545

46+
<!--
47+
A Native AOT publish restores every project for a runtime identifier, which is not what the lock
48+
files record, and would rewrite them. Its lock files go under artifacts/obj instead, where
49+
nothing keeps them, so a publish leaves the working tree as it found it. The `TestAot` pipeline
50+
of build.fsx is what sets `FantomasAot`.
51+
-->
52+
<PropertyGroup Condition="'$(FantomasAot)' == 'true'">
53+
<NuGetLockFilePath>$(MSBuildThisFileDirectory)artifacts/obj/$(MSBuildProjectName)/packages.aot.lock.json</NuGetLockFilePath>
54+
<RestoreLockedMode>false</RestoreLockedMode>
55+
</PropertyGroup>
56+
4657
<!-- Versions -->
4758
<PropertyGroup>
48-
<FCSCommitHash>5e9647f158fe282462aba57ed83ada910a91b90e</FCSCommitHash>
59+
<FCSCommitHash>c3cf92a72ac5da1f9aafacbb03a9d1096012d1ae</FCSCommitHash>
60+
<!--
61+
The GitHub repository the vendored compiler sources are downloaded from. Temporarily a fork, for
62+
the Native AOT fixes of dotnet/fsharp#20658 before they are merged. Back to dotnet/fsharp once
63+
they are.
64+
-->
65+
<FCSRepository>nojaf/fsharp</FCSRepository>
4966
</PropertyGroup>
5067

5168
<PropertyGroup>

‎analyzers/AGENTS.md‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ feedback arrives while you work instead of in review. They are ordinary F# analy
1717
| [`FANTOMAS-OPENS-001`](#fantomas-opens-001) | No `open` nothing in the file uses |
1818
| [`FANTOMAS-PARENS-001`](#fantomas-parens-001) | No parentheses the code parses the same without |
1919
| [`FANTOMAS-SNOBMATCH-001`](#fantomas-snobmatch-001) | No `match` where an `if` would do |
20+
| [`FANTOMAS-PRINTF-001`](#fantomas-printf-001) | No printf in code the tool runs |
2021

2122
## FANTOMAS-PIPEBACK-001
2223

@@ -398,6 +399,56 @@ arms, because the two arms it sees are then not the arms every build sees.
398399
**The reported range is the whole match expression**, since the whole of it is what an `if`
399400
replaces. There is no fix attached, for the reason every other rule here has none.
400401

402+
## FANTOMAS-PRINTF-001
403+
404+
Do not use printf in `src/Fantomas` or `src/Fantomas.Core`, the code the command line tool runs:
405+
406+
```fsharp
407+
span.TotalMilliseconds.ToString("F0", Globalization.CultureInfo.InvariantCulture) + "ms"
408+
```
409+
410+
rather than
411+
412+
```fsharp
413+
$"%.0f{span.TotalMilliseconds}ms"
414+
```
415+
416+
printf specializes its formatters at runtime through `MethodInfo.MakeGenericMethod`, and a Native
417+
AOT build of the tool cannot run that. It does not fail every time, which is what makes it worth a
418+
rule: whether a format survives depends on which instantiations the AOT compiler happened to
419+
generate. `sprintf "%O"` of a range worked in the first AOT build while `sprintf "(%d:%d)"` crashed
420+
the parser, and `profile` crashed on the `%.0f` above. The only reliable rule is not to reach printf.
421+
422+
That means two things. Any function that takes a printf format: `sprintf`, `printfn`, `eprintfn`,
423+
`failwithf`, `kprintf` and the rest. And any interpolated string the compiler does not turn into
424+
`String.Concat`. Since dotnet/fsharp#19971 that is one with a hole whose specifier is anything but a
425+
bare `%s`, `%c`, `%d`, `%i` or `%M`: `%A`, `%O`, `%b`, `%f`, `%x`, and anything with a flag, a width
426+
or a precision, such as `%.0f` or `%5d`. A bare `{x}` and a .NET format such as `{x:N2}` are fine.
427+
428+
The tests, `Fantomas.Client` and the build scripts only ever run on the JIT, where printf works, so
429+
the rule says nothing there. It decides by the folder the file is in, because the editor hands an
430+
analyzer the file and not its project.
431+
432+
**It reads the untyped tree and states the compiler's rule itself.** The typed tree would show the
433+
lowering already done, but it is the analyzer SDK's own compiler that builds it, and FCS 43.12 predates
434+
#19971: it lowers only string holes, so every `{count}` of an `int` would read as printf. Once the SDK
435+
ships an FCS with #19971, the rule can move to the typed tree and report every `PrintfFormat` being
436+
constructed, which needs no rule of its own. Until then, a change to what the compiler lowers means
437+
a change here.
438+
439+
What it cannot see is an interpolated string handed to a function of your own that takes a
440+
`Printf.StringFormat`: that string is a printf format whatever its holes are. Such a function has to
441+
pass the format on to `kprintf` or one of its kin, and that call is reported.
442+
443+
`nothing the tool runs goes through printf` in `Fantomas.Tests` is the same rule checked against
444+
the compiled assemblies rather than the source, and so against whatever the compiler that built them
445+
did. The two report the same places, and the places allowed in one are suppressed in the other.
446+
447+
It reports at **error** severity, so that the full `Analyze`, and CI with it, fails on a new
448+
finding. `AnalyzeChanged` demotes it like every local rule. The few uses that remain are ones the
449+
tool never reaches, each suppressed with the reason beside it. There is no fix attached, for the
450+
reason every other rule here has none.
451+
401452
## Suppressing a finding
402453

403454
Use the SDK's comment syntax rather than reshaping the code around it. The SDK filters the messages

‎analyzers/Fantomas.Analyzers.Tests/Fantomas.Analyzers.Tests.fsproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
<Compile Include="UnusedOpensAnalyzerTests.fs" />
2727
<Compile Include="UnnecessaryParensAnalyzerTests.fs" />
2828
<Compile Include="SnobMatchAnalyzerTests.fs" />
29+
<Compile Include="PrintfAnalyzerTests.fs" />
2930
</ItemGroup>
3031
<ItemGroup>
3132
<ProjectReference Include="..\Fantomas.Analyzers\Fantomas.Analyzers.fsproj" />

‎analyzers/Fantomas.Analyzers.Tests/PrintfAnalyzerTests.fs‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
module Fantomas.Analyzers.Tests.PrintfAnalyzerTests
2+
3+
open NUnit.Framework
4+
open Fantomas.Analyzers.Tests.TestHelpers
5+
open Fantomas.Analyzers.PrintfAnalyzer
6+
7+
let private toolFile: string = "/repository/src/Fantomas/Report.fs"
8+
9+
let private analyzeToolSource (source: string) : FSharp.Analyzers.SDK.Message list =
10+
analyzeSourceAt cliAnalyzer toolFile source
11+
12+
[<Test>]
13+
let ``sprintf is reported`` () =
14+
let source: string =
15+
"""module M
16+
17+
let f (x: int) : string = sprintf "%d items" x"""
18+
19+
analyzeToolSource source |> assertLines [ 3 ]
20+
21+
[<Test>]
22+
let ``failwithf is reported`` () =
23+
let source: string =
24+
"""module M
25+
26+
let f (x: string) : int = failwithf "no such setting: %s" x"""
27+
28+
analyzeToolSource source |> assertLines [ 3 ]
29+
30+
[<Test>]
31+
let ``an interpolated string with a precision is reported`` () =
32+
let source: string =
33+
"""module M
34+
35+
let f (x: float) : string = $"%.0f{x}ms"
36+
"""
37+
38+
analyzeToolSource source |> assertLines [ 3 ]
39+
40+
[<Test>]
41+
let ``an interpolated string with a structured hole is reported`` () =
42+
let source: string =
43+
"""module M
44+
45+
let f (x: int list) : string = $"got %A{x}"
46+
"""
47+
48+
analyzeToolSource source |> assertLines [ 3 ]
49+
50+
// The compiler lowers these to `String.Concat`, so nothing goes through printf.
51+
[<Test>]
52+
let ``an interpolated string with bare and typed holes is not reported`` () =
53+
let source: string =
54+
"""module M
55+
56+
let f (name: string) (count: int) : string = $"%s{name} has %d{count} files, {count} in total"
57+
"""
58+
59+
analyzeToolSource source |> assertLines []
60+
61+
[<Test>]
62+
let ``a hole with a width is reported`` () =
63+
let source: string =
64+
"""module M
65+
66+
let f (x: int) : string = $"[%5d{x}]"
67+
"""
68+
69+
analyzeToolSource source |> assertLines [ 3 ]
70+
71+
[<Test>]
72+
let ``a hole with a lowercase boolean specifier is reported`` () =
73+
let source: string =
74+
"""module M
75+
76+
let f (x: bool) : string = $"flag: %b{x}"
77+
"""
78+
79+
analyzeToolSource source |> assertLines [ 3 ]
80+
81+
// `%%` is an escaped percent sign, so what follows is text and the hole after it is bare.
82+
[<Test>]
83+
let ``an escaped percent sign before a bare hole is not reported`` () =
84+
let source: string =
85+
"""module M
86+
87+
let f (x: int) : string = $"100%%d{x}"
88+
"""
89+
90+
analyzeToolSource source |> assertLines []
91+
92+
[<Test>]
93+
let ``a hole with a dotnet format is not reported`` () =
94+
let source: string =
95+
"""module M
96+
97+
let f (x: float) : string = $"{x:N2} and {x,8}"
98+
"""
99+
100+
analyzeToolSource source |> assertLines []
101+
102+
[<Test>]
103+
let ``kprintf is reported`` () =
104+
let source: string =
105+
"""module M
106+
107+
let f (format: Printf.StringFormat<'T, string>) : 'T = Printf.kprintf id format"""
108+
109+
analyzeToolSource source |> assertLines [ 3 ]
110+
111+
[<Test>]
112+
let ``sprintf passed as a value is reported`` () =
113+
let source: string =
114+
"""module M
115+
116+
let f (xs: int list) : string list = List.map (sprintf "%d") xs"""
117+
118+
analyzeToolSource source |> assertLines [ 3 ]
119+
120+
[<Test>]
121+
let ``printf inside a lambda is reported`` () =
122+
let source: string =
123+
"""module M
124+
125+
let f (xs: int list) : string list =
126+
xs |> List.map (fun x -> sprintf "%i" x)"""
127+
128+
analyzeToolSource source |> assertLines [ 4 ]
129+
130+
[<Test>]
131+
let ``printf in Fantomas.Core is reported`` () =
132+
let source: string =
133+
"""module M
134+
135+
let f (x: int) : string = sprintf "%d" x"""
136+
137+
analyzeSourceAt cliAnalyzer "/repository/src/Fantomas.Core/Utils.fs" source
138+
|> assertLines [ 3 ]
139+
140+
// The tests and Fantomas.Client only run on the JIT, where printf is fine.
141+
[<TestCase("/repository/src/Fantomas.Tests/ReportTests.fs")>]
142+
[<TestCase("/repository/src/Fantomas.Client/LSPFantomasService.fs")>]
143+
[<TestCase("/repository/src/Fantomas.Core.Tests/CodePrinterTests.fs")>]
144+
let ``printf outside the code the tool runs is not reported`` (fileName: string) =
145+
let source: string =
146+
"""module M
147+
148+
let f (x: int) : string = sprintf "%d" x"""
149+
150+
analyzeSourceAt cliAnalyzer fileName source |> assertLines []

‎analyzers/Fantomas.Analyzers.Tests/TestHelpers.fs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,17 @@ let analyzeSource (analyzer: Analyzer<CliContext>) (source: string) : Message li
5454
/// Builds a context over a signature file and its implementation, analyzing whichever of the two
5555
/// `target` names. A signature file cannot be type checked on its own, so both are always compiled
5656
/// and only the file under analysis changes.
57+
/// Analyze a snippet as the file at `fileName`, for a rule that decides by where a file lives.
58+
let analyzeSourceAt (analyzer: Analyzer<CliContext>) (fileName: string) (source: string) : Message list =
59+
let file: SourceFile = { FileName = fileName; Source = source }
60+
61+
let ctx: CliContext =
62+
getContextFor (AnalyzerProjectOptions.BackgroundCompilerOptions projectOptions) [ file ] file
63+
|> Async.AwaitTask
64+
|> Async.RunSynchronously
65+
66+
analyzer ctx |> Async.RunSynchronously
67+
5768
let private contextForPair
5869
(signature: string)
5970
(implementation: string)

‎analyzers/Fantomas.Analyzers/Fantomas.Analyzers.fsproj‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@
4747
<Compile Include="UnnecessaryParensAnalyzer.fs" />
4848
<Compile Include="SnobMatchAnalyzer.fsi" />
4949
<Compile Include="SnobMatchAnalyzer.fs" />
50+
<Compile Include="PrintfAnalyzer.fsi" />
51+
<Compile Include="PrintfAnalyzer.fs" />
5052
</ItemGroup>
5153
<ItemGroup>
5254
<!-- Must track the fsharp-analyzers version pinned in .config/dotnet-tools.json. -->

0 commit comments

Comments
 (0)