From 98286e7ca06911c31599bd0c12892a8ac138b988 Mon Sep 17 00:00:00 2001 From: Mangel Maxime Date: Thu, 16 Jul 2026 21:00:49 +0200 Subject: [PATCH 1/7] feat(all): support suppressing warnings via // fable-disable comments --- src/Fable.Cli/Main.fs | 5 +- src/Fable.Compiler/Library.fs | 6 +- src/Fable.Transforms/Babel/Fable2Babel.fs | 4 +- src/Fable.Transforms/Beam/Fable2Beam.fs | 4 +- src/Fable.Transforms/Dart/Fable2Dart.fs | 4 +- src/Fable.Transforms/Dart/Replacements.fs | 3 +- src/Fable.Transforms/FSharp2Fable.fs | 4 +- src/Fable.Transforms/Fable.Transforms.fsproj | 2 + src/Fable.Transforms/Global/Compiler.fs | 3 +- src/Fable.Transforms/Global/WarningCodes.fs | 23 ++ .../Global/WarningSuppression.fs | 231 ++++++++++++++++++ src/Fable.Transforms/Php/Fable2Php.fs | 4 +- src/Fable.Transforms/Python/PythonCompiler.fs | 4 +- src/Fable.Transforms/Python/Replacements.fs | 4 +- src/Fable.Transforms/Replacements.fs | 4 +- src/Fable.Transforms/Rust/Fable2Rust.fs | 4 +- src/Fable.Transforms/State.fs | 52 +++- src/Fable.Transforms/Transforms.Util.fs | 20 +- .../src/Fable.Standalone.fsproj | 2 + .../Compiler/CompilerMessagesTests.fs | 91 +++++++ 20 files changed, 443 insertions(+), 31 deletions(-) create mode 100644 src/Fable.Transforms/Global/WarningCodes.fs create mode 100644 src/Fable.Transforms/Global/WarningSuppression.fs diff --git a/src/Fable.Cli/Main.fs b/src/Fable.Cli/Main.fs index 9adcfd7878..c88307ab1c 100644 --- a/src/Fable.Cli/Main.fs +++ b/src/Fable.Cli/Main.fs @@ -356,6 +356,8 @@ type FsWatcher(delayMs: int) = type ProjectCracked(cliArgs: CliArgs, crackerResponse: CrackerResponse, sourceFiles: Fable.Compiler.File array) = + let sourceReader = lazy (snd (Fable.Compiler.File.MakeSourceReader sourceFiles)) + member _.CliArgs = cliArgs member _.ProjectFile = cliArgs.ProjectFile member _.FableOptions = cliArgs.CompilerOptions @@ -392,7 +394,8 @@ type ProjectCracked(cliArgs: CliArgs, crackerResponse: CrackerResponse, sourceFi fableLibDir, crackerResponse.OutputType, ?outDir = cliArgs.OutDir, - ?watchDependencies = watchDependencies + ?watchDependencies = watchDependencies, + sourceReader = sourceReader.Value ) member _.MapSourceFiles(f) = diff --git a/src/Fable.Compiler/Library.fs b/src/Fable.Compiler/Library.fs index e7b6c7c6df..427789aab0 100644 --- a/src/Fable.Compiler/Library.fs +++ b/src/Fable.Compiler/Library.fs @@ -180,7 +180,8 @@ module CodeServices = opts, fableLibDir, crackerResponse.OutputType, - ?outDir = cliArgs.OutDir + ?outDir = cliArgs.OutDir, + sourceReader = sourceReader ) // TODO: make it configurable if FableTransforms.transformFile is applied? @@ -319,7 +320,8 @@ module CodeServices = opts, fableLibDir, crackerResponse.OutputType, - ?outDir = cliArgs.OutDir + ?outDir = cliArgs.OutDir, + sourceReader = sourceReader ) let outputPath = Path.ChangeExtension(currentFile, ".js") diff --git a/src/Fable.Transforms/Babel/Fable2Babel.fs b/src/Fable.Transforms/Babel/Fable2Babel.fs index e9312c0808..3fe120a952 100644 --- a/src/Fable.Transforms/Babel/Fable2Babel.fs +++ b/src/Fable.Transforms/Babel/Fable2Babel.fs @@ -4987,8 +4987,8 @@ module Compiler = member _.AddWatchDependency(fileName) = com.AddWatchDependency(fileName) - member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string) = - com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag) + member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string, ?code: string) = + com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag, ?code = code) let makeCompiler com = BabelCompiler(com) diff --git a/src/Fable.Transforms/Beam/Fable2Beam.fs b/src/Fable.Transforms/Beam/Fable2Beam.fs index 2f49d7df79..a5ac265d2b 100644 --- a/src/Fable.Transforms/Beam/Fable2Beam.fs +++ b/src/Fable.Transforms/Beam/Fable2Beam.fs @@ -3760,8 +3760,8 @@ let transformFile (com: Fable.Compiler) (file: File) : Beam.ErlModule = member _.GetInlineExpr(key) = com.GetInlineExpr(key) member _.AddWatchDependency(file) = com.AddWatchDependency(file) - member _.AddLog(msg, severity, ?range, ?fileName, ?tag) = - com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag) + member _.AddLog(msg, severity, ?range, ?fileName, ?tag, ?code) = + com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag, ?code = code) } let forms = diff --git a/src/Fable.Transforms/Dart/Fable2Dart.fs b/src/Fable.Transforms/Dart/Fable2Dart.fs index 788a68ad0b..26114be2d6 100644 --- a/src/Fable.Transforms/Dart/Fable2Dart.fs +++ b/src/Fable.Transforms/Dart/Fable2Dart.fs @@ -3117,8 +3117,8 @@ module Compiler = member _.AddWatchDependency(fileName) = com.AddWatchDependency(fileName) - member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string) = - com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag) + member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string, ?code: string) = + com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag, ?code = code) let makeCompiler com = DartCompiler(com) diff --git a/src/Fable.Transforms/Dart/Replacements.fs b/src/Fable.Transforms/Dart/Replacements.fs index aed42ea52d..6fcf3bdbdc 100644 --- a/src/Fable.Transforms/Dart/Replacements.fs +++ b/src/Fable.Transforms/Dart/Replacements.fs @@ -1395,7 +1395,8 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt | "GetEnumerator", Some c, _ -> stringToCharSeq c |> getEnumerator com r t |> Some | ("Contains" | "StartsWith" | "EndsWith" as meth), Some c, arg :: _ -> if List.isMultiple args then - addWarning com ctx.InlinePath r $"String.%s{meth}: second argument is ignored" + WarningCodes.stringSecondArgumentIgnored meth + |> addWarningWithCode com ctx.InlinePath r Helper.InstanceCall(c, Naming.lowerFirst meth, t, [ arg ], ?loc = r) |> Some | ReplaceName [ "ToUpper", "toUpperCase" diff --git a/src/Fable.Transforms/FSharp2Fable.fs b/src/Fable.Transforms/FSharp2Fable.fs index 4e25f6b74c..49bd3c3607 100644 --- a/src/Fable.Transforms/FSharp2Fable.fs +++ b/src/Fable.Transforms/FSharp2Fable.fs @@ -2857,8 +2857,8 @@ type FableCompiler(com: Compiler) = member _.GetInlineExpr(fullName) = com.GetInlineExpr(fullName) member _.AddWatchDependency(fileName) = com.AddWatchDependency(fileName) - member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string) = - com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag) + member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string, ?code: string) = + com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag, ?code = code) let rec attachClassMembers (com: FableCompiler) = diff --git a/src/Fable.Transforms/Fable.Transforms.fsproj b/src/Fable.Transforms/Fable.Transforms.fsproj index 738cab2bb8..d2a0d0331b 100644 --- a/src/Fable.Transforms/Fable.Transforms.fsproj +++ b/src/Fable.Transforms/Fable.Transforms.fsproj @@ -5,6 +5,8 @@ + + diff --git a/src/Fable.Transforms/Global/Compiler.fs b/src/Fable.Transforms/Global/Compiler.fs index 7b2b2ac975..cb33b5e9a1 100644 --- a/src/Fable.Transforms/Global/Compiler.fs +++ b/src/Fable.Transforms/Global/Compiler.fs @@ -86,7 +86,8 @@ type Compiler = abstract AddWatchDependency: file: string -> unit abstract AddLog: - msg: string * severity: Severity * ?range: SourceLocation * ?fileName: string * ?tag: string -> unit + msg: string * severity: Severity * ?range: SourceLocation * ?fileName: string * ?tag: string * ?code: string -> + unit type InlineExprLazy(f: Compiler -> InlineExpr) = let mutable value: InlineExpr voption = ValueNone diff --git a/src/Fable.Transforms/Global/WarningCodes.fs b/src/Fable.Transforms/Global/WarningCodes.fs new file mode 100644 index 0000000000..002e5b9be2 --- /dev/null +++ b/src/Fable.Transforms/Global/WarningCodes.fs @@ -0,0 +1,23 @@ +/// Central registry of stable codes + messages for `addWarningWithCode`: one function per +/// warning, so call sites sharing the same warning (e.g. StartsWith/EndsWith, JS/Python) can't +/// drift into different codes or wording. Codes are never reused/renumbered once published. +/// Usage: `WarningCodes.someWarning arg1 arg2 |> addWarningWithCode com inlinePath range`. +module Fable.Transforms.WarningCodes + +[] +let private CultureInfoIgnored = "FABLE0001" + +[] +let private StringSecondArgumentIgnored = "FABLE0002" + +/// `String.StartsWith`/`EndsWith` with a `CultureInfo` argument: the comparison always runs +/// with the target's default culture rules, the argument is accepted but has no effect. +/// Used in both the JS/TS and Python replacements. +let cultureInfoIgnored () = + CultureInfoIgnored, "CultureInfo argument is ignored" + +/// `String.Contains`/`StartsWith`/`EndsWith` called with a `StringComparison` argument on the +/// Dart target: only the comparison itself is honored, `methodName` fills in which one so the +/// message stays specific (e.g. "String.Contains: second argument is ignored"). +let stringSecondArgumentIgnored (methodName: string) = + StringSecondArgumentIgnored, $"String.{methodName}: second argument is ignored" diff --git a/src/Fable.Transforms/Global/WarningSuppression.fs b/src/Fable.Transforms/Global/WarningSuppression.fs new file mode 100644 index 0000000000..8a4307e481 --- /dev/null +++ b/src/Fable.Transforms/Global/WarningSuppression.fs @@ -0,0 +1,231 @@ +/// Computes which diagnostics `// fable-disable/-enable...` comments suppress (ESLint's +/// disable-line/next-line/block model), via real comment tokens - not raw text matching. +module Fable.Transforms.WarningSuppression + +open System.Text.RegularExpressions +open FSharp.Compiler.Tokenization + +type private BlockState = + | NoneDisabled + | AllDisabledExcept of Set + | SpecificDisabled of Set + +type private Directive = + | DisableLine of codes: Set option * line: int + | DisableNextLine of codes: Set option * line: int + | Disable of codes: Set option * line: int + | Enable of codes: Set option * line: int + +let private directiveRegex = + Regex(@"^fable-(disable-next-line|disable-line|disable|enable)(?:\s+(.+))?$", RegexOptions.Compiled) + +let private parseCodes (s: string) = + let codes = + s.Split([| ' '; ','; '\t' |], System.StringSplitOptions.RemoveEmptyEntries) + |> Set.ofArray + + if Set.isEmpty codes then + None + else + Some codes + +let private stripCommentMarkers (raw: string) = + let raw = + if raw.StartsWith("//", System.StringComparison.Ordinal) then + raw.Substring(2) + elif raw.StartsWith("(*", System.StringComparison.Ordinal) then + raw.Substring(2) + else + raw + + let raw = + if raw.EndsWith("*)", System.StringComparison.Ordinal) then + raw.Substring(0, raw.Length - 2) + else + raw + + raw.Trim() + +let private tryParseDirective (line: int) (commentText: string) : Directive option = + let text = stripCommentMarkers commentText + let m = directiveRegex.Match(text) + + if not m.Success then + None + else + let codes = + if m.Groups[2].Success then + parseCodes m.Groups[2].Value + else + None + + match m.Groups[1].Value with + | "disable-line" -> Some(DisableLine(codes, line)) + | "disable-next-line" -> Some(DisableNextLine(codes, line)) + | "disable" -> Some(Disable(codes, line)) + | "enable" -> Some(Enable(codes, line)) + | _ -> None + +/// Gathers each line's comment token runs as plain text (a line can have more than one, e.g. a +/// block comment then a trailing line comment); also returns the lexer state to carry into the +/// next line, needed to resume correctly inside multi-line block comments/strings. +let private scanLineComments + (tokenizer: FSharpLineTokenizer) + (initialState: FSharpTokenizerLexState) + (line: string) + : string list * FSharpTokenizerLexState + = + // Each finished run is one comment on the line; `current` is the run being built. + let runs = ResizeArray() + let mutable current: System.Text.StringBuilder option = None + + // Pull tokens one at a time, threading the lexer state (needed across lines too). + let rec loop state = + match tokenizer.ScanToken(state) with + | Some(tok: FSharpTokenInfo), state2 -> + if tok.ColorClass = FSharpTokenColorKind.Comment then + let text = line.Substring(tok.LeftColumn, tok.RightColumn - tok.LeftColumn + 1) + + match current with + // Still inside the same comment: glue this token onto the current run. + | Some sb -> sb.Append(text) |> ignore + // First comment token after non-comment text: start a new run. + | None -> + let sb = System.Text.StringBuilder(text: string) + current <- Some sb + runs.Add(sb) + else + // Non-comment token: close the current run, if any (e.g. code between two comments). + current <- None + + loop state2 + // No more tokens on this line: return the final state for the next line. + | None, state2 -> state2 + + let endState = loop initialState + // Materialize each run's text; endState lets the caller resume correctly on the next line. + (runs |> Seq.map string |> List.ofSeq), endState + +/// Computed, queryable suppression info for a single source file. +type FileSuppressions = + private + { + /// 1-based line -> codes suppressed specifically on that line (None = all codes) + LineOnly: Map option> + /// index (line - 1) -> block-disable state as of (and including) that line + BlockAtLine: BlockState[] + } + + /// Is a diagnostic with the given code (None = no code assigned to it) suppressed on this line? + member this.IsSuppressed(line: int, code: string option) = + let lineSuppressed = + match Map.tryFind line this.LineOnly with + | Some None -> true + | Some(Some codes) -> code |> Option.map codes.Contains |> Option.defaultValue false + | None -> false + + let blockSuppressed = + if line < 1 || line > this.BlockAtLine.Length then + false + else + match this.BlockAtLine[line - 1] with + | NoneDisabled -> false + | AllDisabledExcept enabled -> + match code with + | None -> true + | Some c -> not (Set.contains c enabled) + | SpecificDisabled disabled -> + match code with + | None -> false + | Some c -> Set.contains c disabled + + lineSuppressed || blockSuppressed + + static member Empty = + { + LineOnly = Map.empty + BlockAtLine = [||] + } + +let private transition (state: BlockState) (codes: Set option) (isDisable: bool) = + match isDisable, codes, state with + | true, None, _ -> AllDisabledExcept Set.empty + | true, Some codes, NoneDisabled -> SpecificDisabled codes + | true, Some codes, SpecificDisabled s -> SpecificDisabled(Set.union s codes) + | true, Some codes, AllDisabledExcept ex -> AllDisabledExcept(Set.difference ex codes) + | false, None, _ -> NoneDisabled + | false, Some _, NoneDisabled -> NoneDisabled + | false, Some codes, SpecificDisabled s -> SpecificDisabled(Set.difference s codes) + | false, Some codes, AllDisabledExcept ex -> AllDisabledExcept(Set.union ex codes) + +/// Scans the given source text for `fable-disable*`/`fable-enable*` comments and builds a +/// queryable `FileSuppressions` snapshot. Meant to be computed once per file and cached. +let compute (source: string) : FileSuppressions = + let lines = source.Replace("\r\n", "\n").Split('\n') + + if lines.Length = 0 then + FileSuppressions.Empty + else + let sourceTok = FSharpSourceTokenizer([], None, None, None) + let directives = ResizeArray() + let mutable state = FSharpTokenizerLexState.Initial + + for i in 0 .. lines.Length - 1 do + let lineNo = i + 1 + let tokenizer = sourceTok.CreateLineTokenizer(lines[i]) + let runs, newState = scanLineComments tokenizer state lines[i] + state <- newState + + for run in runs do + tryParseDirective lineNo run |> Option.iter directives.Add + + let lineOnly = + (Map.empty, directives) + ||> Seq.fold (fun acc d -> + let merge line (codes: Set option) = + let merged = + match Map.tryFind line acc, codes with + | Some None, _ + | _, None -> None + | None, Some c -> Some c + | Some(Some existing), Some c -> Some(Set.union existing c) + + Map.add line merged acc + + match d with + | DisableLine(codes, line) -> merge line codes + | DisableNextLine(codes, line) -> merge (line + 1) codes + | Disable _ + | Enable _ -> acc + ) + + let blockDirectivesByLine = + directives + |> Seq.choose ( + function + | Disable(codes, line) -> Some(line, codes, true) + | Enable(codes, line) -> Some(line, codes, false) + | DisableLine _ + | DisableNextLine _ -> None + ) + |> Seq.groupBy (fun (line, _, _) -> line) + |> Map.ofSeq + + // Filling this with hundreds/thousands of NoneDisabled is nearly free: F# compiles a + // nullary DU case to a singleton, so every slot is a pointer to the same object. + let blockAtLine = Array.create lines.Length NoneDisabled + let mutable current = NoneDisabled + + for lineNo in 1 .. lines.Length do + match Map.tryFind lineNo blockDirectivesByLine with + | Some ds -> + for _, codes, isDisable in ds do + current <- transition current codes isDisable + | None -> () + + blockAtLine[lineNo - 1] <- current + + { + LineOnly = lineOnly + BlockAtLine = blockAtLine + } diff --git a/src/Fable.Transforms/Php/Fable2Php.fs b/src/Fable.Transforms/Php/Fable2Php.fs index 4afbb5ee5b..2aa0c1970c 100644 --- a/src/Fable.Transforms/Php/Fable2Php.fs +++ b/src/Fable.Transforms/Php/Fable2Php.fs @@ -2138,8 +2138,8 @@ type PhpCompiler(com: Fable.Compiler) = member this.WillPrecompileInlineFunction(file) = com.WillPrecompileInlineFunction(file) - member this.AddLog(msg, severity, rang, fileName, tag) = - com.AddLog(msg, severity, ?range = rang, ?fileName = fileName, ?tag = tag) + member this.AddLog(msg, severity, rang, fileName, tag, code) = + com.AddLog(msg, severity, ?range = rang, ?fileName = fileName, ?tag = tag, ?code = code) member this.AddWatchDependency(file) = com.AddWatchDependency(file) diff --git a/src/Fable.Transforms/Python/PythonCompiler.fs b/src/Fable.Transforms/Python/PythonCompiler.fs index 63563c02bf..7430a9a1e5 100644 --- a/src/Fable.Transforms/Python/PythonCompiler.fs +++ b/src/Fable.Transforms/Python/PythonCompiler.fs @@ -137,8 +137,8 @@ type PythonCompiler(com: Compiler) = member _.AddWatchDependency(fileName) = com.AddWatchDependency(fileName) - member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string) = - com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag) + member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string, ?code: string) = + com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag, ?code = code) let makeCompiler com = PythonCompiler(com) diff --git a/src/Fable.Transforms/Python/Replacements.fs b/src/Fable.Transforms/Python/Replacements.fs index 5676b8c46c..8e3d24738c 100644 --- a/src/Fable.Transforms/Python/Replacements.fs +++ b/src/Fable.Transforms/Python/Replacements.fs @@ -1512,7 +1512,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt Helper.LibCall(com, "string", "starts_with", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) |> Some | "StartsWith", Some c, [ value; ignoreCase; _culture ] -> - addWarning com ctx.InlinePath r "CultureInfo argument is ignored" + WarningCodes.cultureInfoIgnored () |> addWarningWithCode com ctx.InlinePath r let args = [ value; ignoreCase ] Helper.LibCall(com, "string", "starts_with", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) @@ -1524,7 +1524,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt Helper.LibCall(com, "string", "ends_with", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) |> Some | "EndsWith", Some c, [ value; ignoreCase; _culture ] -> - addWarning com ctx.InlinePath r "CultureInfo argument is ignored" + WarningCodes.cultureInfoIgnored () |> addWarningWithCode com ctx.InlinePath r let args = [ value; ignoreCase ] Helper.LibCall(com, "string", "ends_with", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) diff --git a/src/Fable.Transforms/Replacements.fs b/src/Fable.Transforms/Replacements.fs index aef89aa1e7..7b12349c53 100644 --- a/src/Fable.Transforms/Replacements.fs +++ b/src/Fable.Transforms/Replacements.fs @@ -1723,7 +1723,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt Helper.LibCall(com, "String", "startsWith", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) |> Some | "StartsWith", Some c, [ value; ignoreCase; _culture ] -> - addWarning com ctx.InlinePath r "CultureInfo argument is ignored" + WarningCodes.cultureInfoIgnored () |> addWarningWithCode com ctx.InlinePath r let args = [ value; ignoreCase ] Helper.LibCall(com, "String", "startsWith", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) @@ -1733,7 +1733,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt Helper.LibCall(com, "String", "endsWith", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) |> Some | "EndsWith", Some c, [ value; ignoreCase; _culture ] -> - addWarning com ctx.InlinePath r "CultureInfo argument is ignored" + WarningCodes.cultureInfoIgnored () |> addWarningWithCode com ctx.InlinePath r let args = [ value; ignoreCase ] Helper.LibCall(com, "String", "endsWith", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) diff --git a/src/Fable.Transforms/Rust/Fable2Rust.fs b/src/Fable.Transforms/Rust/Fable2Rust.fs index 58aeba6934..f5f846c6d3 100644 --- a/src/Fable.Transforms/Rust/Fable2Rust.fs +++ b/src/Fable.Transforms/Rust/Fable2Rust.fs @@ -5735,8 +5735,8 @@ module Compiler = member _.GetInlineExpr(fullName) = com.GetInlineExpr(fullName) member _.AddWatchDependency(fileName) = com.AddWatchDependency(fileName) - member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string) = - com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag) + member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string, ?code: string) = + com.AddLog(msg, severity, ?range = range, ?fileName = fileName, ?tag = tag, ?code = code) let makeCompiler com = RustCompiler(com) diff --git a/src/Fable.Transforms/State.fs b/src/Fable.Transforms/State.fs index 65672dec4a..448d9ca3e8 100644 --- a/src/Fable.Transforms/State.fs +++ b/src/Fable.Transforms/State.fs @@ -257,15 +257,17 @@ type LogEntry = Severity: Severity Range: SourceLocation option FileName: string option + Code: string option } - static member Make(severity, msg, ?fileName, ?range, ?tag) = + static member Make(severity, msg, ?fileName, ?range, ?tag, ?code) = { Message = msg Tag = defaultArg tag "FABLE" Severity = severity Range = range FileName = fileName + Code = code } static member MakeError(msg, ?fileName, ?range, ?tag) = @@ -283,7 +285,8 @@ type CompilerImpl ?outDir: string, ?watchDependencies: HashSet, ?logs: ResizeArray, - ?isPrecompilingInlineFunction: bool + ?isPrecompilingInlineFunction: bool, + ?sourceReader: SourceReader ) = @@ -291,6 +294,23 @@ type CompilerImpl let outType = defaultArg outType OutputType.Exe let logs = Option.defaultWith ResizeArray logs let fableLibraryDir = fableLibDir.TrimEnd('/') + let suppressionsCache = Dictionary() + + let getSuppressions fileName = + match suppressionsCache.TryGetValue(fileName) with + | true, s -> s + | false, _ -> + let suppressions = + match sourceReader with + | None -> WarningSuppression.FileSuppressions.Empty + | Some read -> + try + (snd (read fileName)).Value |> WarningSuppression.compute + with _ -> + WarningSuppression.FileSuppressions.Empty + + suppressionsCache[fileName] <- suppressions + suppressions member _.Logs = logs.ToArray() @@ -333,7 +353,8 @@ type CompilerImpl ?outDir = outDir, ?watchDependencies = watchDependencies, logs = logs, - isPrecompilingInlineFunction = true + isPrecompilingInlineFunction = true, + ?sourceReader = sourceReader ) member _.GetImplementationFile(fileName) = @@ -387,6 +408,25 @@ type CompilerImpl | Some watchDependencies when file <> currentFile -> watchDependencies.Add(file) |> ignore | _ -> () - member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string) = - LogEntry.Make(severity, msg, ?range = range, ?fileName = fileName, ?tag = tag) - |> logs.Add + member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string, ?code: string) = + // Only warnings can be suppressed, errors always surface (matches F#'s own #nowarn) + let isSuppressed = + severity = Severity.Warning + && ( + match range with + | None -> false + | Some(r: SourceLocation) -> + let file = + match fileName with + | Some f -> f + | None -> + match r.File with + | Some f -> f + | None -> currentFile + + (getSuppressions file).IsSuppressed(r.start.line, code) + ) + + if not isSuppressed then + LogEntry.Make(severity, msg, ?range = range, ?fileName = fileName, ?tag = tag, ?code = code) + |> logs.Add diff --git a/src/Fable.Transforms/Transforms.Util.fs b/src/Fable.Transforms/Transforms.Util.fs index c5cc988a78..9d796cdede 100644 --- a/src/Fable.Transforms/Transforms.Util.fs +++ b/src/Fable.Transforms/Transforms.Util.fs @@ -668,7 +668,14 @@ module Log = FromRange: SourceLocation option } - let private addLog (com: Compiler) (inlinePath: InlinePath list) (range: SourceLocation option) msg severity = + let private addLogWithCode + (com: Compiler) + (inlinePath: InlinePath list) + (range: SourceLocation option) + msg + severity + (code: string option) + = let printInlineSource fromPath (p: InlinePath) = let path = Path.getRelativeFileOrDirPath false fromPath false p.FromFile @@ -685,11 +692,20 @@ module Log = file, msg + " - Inline call from " + inlinePath | [] -> range |> Option.bind (fun r -> r.File) |> Option.defaultValue com.CurrentFile, msg - com.AddLog(msg, severity, ?range = range, fileName = actualFile) + com.AddLog(msg, severity, ?range = range, fileName = actualFile, ?code = code) + + let private addLog (com: Compiler) (inlinePath: InlinePath list) (range: SourceLocation option) msg severity = + addLogWithCode com inlinePath range msg severity None let addWarning (com: Compiler) inlinePath range warning = addLog com inlinePath range warning Severity.Warning + /// Same as `addWarning`, but tags the warning with a stable code (e.g. "FABLE0001") so it + /// can be suppressed via `// fable-disable-line/-next-line/-enable CODE` comments. + /// Meant to be used as `WarningCodes.someWarning arg1 arg2 |> addWarningWithCode com inlinePath range`. + let addWarningWithCode (com: Compiler) inlinePath range ((code, warning): string * string) = + addLogWithCode com inlinePath range warning Severity.Warning (Some code) + let addError (com: Compiler) inlinePath range error = addLog com inlinePath range error Severity.Error diff --git a/src/fable-standalone/src/Fable.Standalone.fsproj b/src/fable-standalone/src/Fable.Standalone.fsproj index 64ac4fa020..f6bc334cd9 100644 --- a/src/fable-standalone/src/Fable.Standalone.fsproj +++ b/src/fable-standalone/src/Fable.Standalone.fsproj @@ -22,6 +22,8 @@ + + diff --git a/tests/Integration/Compiler/CompilerMessagesTests.fs b/tests/Integration/Compiler/CompilerMessagesTests.fs index 24420f1bd1..402bd7f674 100644 --- a/tests/Integration/Compiler/CompilerMessagesTests.fs +++ b/tests/Integration/Compiler/CompilerMessagesTests.fs @@ -125,4 +125,95 @@ type MyClass() = compile source |> Assert.Is.success |> ignore + + testCase "CultureInfo argument warning is not suppressed by default" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Exists.warningWith "CultureInfo argument is ignored" + |> ignore + + testCase "The same code covers both StartsWith and EndsWith call sites" <| fun _ -> + // Regression test for a real bug: StartsWith and EndsWith both raise the same logical + // "CultureInfo argument is ignored" warning from two separate call sites in + // Replacements.fs, sharing WarningCodes.CultureInfoIgnored. Both must be suppressible + // by one code, and (in Python's Replacements.fs) this used to have no code at all. + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 +"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 +""" + compile source + |> Assert.Are.warnings 0 + |> ignore + + testCase "fable-disable-line suppresses a warning on the same line" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 +""" + compile source + |> Assert.Are.warnings 0 + |> ignore + + testCase "fable-disable-next-line suppresses a warning on the following line" <| fun _ -> + let source = + """ +open System.Globalization +// fable-disable-next-line FABLE0001 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Are.warnings 0 + |> ignore + + testCase "fable-disable/fable-enable suppresses warnings in a block" <| fun _ -> + let source = + """ +open System.Globalization +// fable-disable FABLE0001 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore +// fable-enable FABLE0001 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Are.warnings 1 + |> ignore + + testCase "A mismatched code does not suppress the warning" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line SOME_OTHER_CODE +""" + compile source + |> Assert.Exists.warningWith "CultureInfo argument is ignored" + |> ignore + + testCase "A bare fable-disable-line suppresses regardless of code" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line +""" + compile source + |> Assert.Are.warnings 0 + |> ignore + + testCase "A string literal that looks like a directive is not treated as one" <| fun _ -> + let source = + """ +open System.Globalization +let s = "// fable-disable-line FABLE0001" +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Exists.warningWith "CultureInfo argument is ignored" + |> ignore ] From 899568a38448b69211205f69e2a0127c081b8130 Mon Sep 17 00:00:00 2001 From: Mangel Maxime Date: Thu, 13 Aug 2026 15:22:31 +0200 Subject: [PATCH 2/7] fix: take into account feedbacks from other maintainers --- src/Fable.Cli/Main.fs | 53 +- src/Fable.Compiler/Library.fs | 17 +- src/Fable.Transforms/Fable.Transforms.fsproj | 2 +- src/Fable.Transforms/Global/WarningCodes.fs | 45 +- .../Global/WarningSuppression.fs | 480 +++++++++++++----- src/Fable.Transforms/Python/Replacements.fs | 4 +- src/Fable.Transforms/Replacements.fs | 4 +- src/Fable.Transforms/State.fs | 26 +- src/Fable.Transforms/Transforms.Util.fs | 2 + .../src/Fable.Standalone.fsproj | 2 +- src/fable-standalone/src/Main.fs | 1 + .../Compiler/CompilerMessagesTests.fs | 216 +++++++- tests/Integration/Compiler/Util/Compiler.fs | 16 +- 13 files changed, 708 insertions(+), 160 deletions(-) diff --git a/src/Fable.Cli/Main.fs b/src/Fable.Cli/Main.fs index c88307ab1c..711b88e5f4 100644 --- a/src/Fable.Cli/Main.fs +++ b/src/Fable.Cli/Main.fs @@ -15,7 +15,7 @@ open Fable.Transforms.State open Fable.Compiler.ProjectCracker open Fable.Compiler.Util -module private Util = +module Util = type PathResolver with static member Dummy = @@ -78,10 +78,18 @@ module private Util = | Severity.Error -> "error" | Severity.Info -> "info" + // MSBuild-style ` `: without the code there is no way to + // discover what to put in a `// fable-disable-line` comment, and rendering it here + // also makes Fable warnings parseable by IDEs and MSBuild loggers. + let tag = + match log.Code with + | Some code -> $"%s{log.Tag} %s{code}" + | None -> log.Tag + match log.Range with | Some r -> - $"%s{file}(%i{r.start.line},%i{r.start.column}): (%i{r.``end``.line},%i{r.``end``.column}) %s{severity} %s{log.Tag}: %s{log.Message}" - | None -> $"%s{file}(1,1): %s{severity} %s{log.Tag}: %s{log.Message}" + $"%s{file}(%i{r.start.line},%i{r.start.column}): (%i{r.``end``.line},%i{r.``end``.column}) %s{severity} %s{tag}: %s{log.Message}" + | None -> $"%s{file}(1,1): %s{severity} %s{tag}: %s{log.Message}" let logErrors rootDir (logs: LogEntry seq) = logs @@ -358,6 +366,16 @@ type ProjectCracked(cliArgs: CliArgs, crackerResponse: CrackerResponse, sourceFi let sourceReader = lazy (snd (Fable.Compiler.File.MakeSourceReader sourceFiles)) + // Shared by every file of the project: a warning can point at any file (inlined calls) and + // files compile in parallel. A new ProjectCracked is built on every watch rebuild, so this + // never outlives the source it was computed from. + let warningSuppression = + lazy + (WarningSuppression.Resolver.FromCompilerOptions( + crackerResponse.ProjectOptions.OtherOptions, + sourceReader.Value + )) + member _.CliArgs = cliArgs member _.ProjectFile = cliArgs.ProjectFile member _.FableOptions = cliArgs.CompilerOptions @@ -395,8 +413,30 @@ type ProjectCracked(cliArgs: CliArgs, crackerResponse: CrackerResponse, sourceFi crackerResponse.OutputType, ?outDir = cliArgs.OutDir, ?watchDependencies = watchDependencies, - sourceReader = sourceReader.Value + warningSuppression = warningSuppression.Value + ) + + /// Problems with the `fable-disable` directives themselves (typo'd codes, directives that + /// suppress nothing). Only valid once every file has been compiled, since a directive in one + /// file can be what suppresses a warning raised while compiling another. + member _.DirectiveDiagnostics(files: string seq) = + warningSuppression.Value.GetDiagnostics(files) + |> List.map (fun (file, d) -> + let pos: Position = + { + line = d.Line + column = 0 + } + + LogEntry.Make( + Severity.Warning, + d.Message, + fileName = file, + range = SourceLocation.Create(pos, pos, file), + code = d.Code + ) ) + |> Array.ofList member _.MapSourceFiles(f) = ProjectCracked(cliArgs, crackerResponse, Array.map f sourceFiles) @@ -1447,6 +1487,11 @@ let private compilationCycle (state: State) (changes: ISet) = Array.append logs [| log |], deps ) + // Every file is compiled by now, so a directive that still hasn't suppressed anything + // really is unused - checking earlier would flag directives that only ever fire for a + // warning raised while some later file was being compiled. + let logs = Array.append logs (projCracked.DirectiveDiagnostics filesToCompile) + let state = { state with PendingFiles = [||] diff --git a/src/Fable.Compiler/Library.fs b/src/Fable.Compiler/Library.fs index 427789aab0..600556081c 100644 --- a/src/Fable.Compiler/Library.fs +++ b/src/Fable.Compiler/Library.fs @@ -181,7 +181,11 @@ module CodeServices = fableLibDir, crackerResponse.OutputType, ?outDir = cliArgs.OutDir, - sourceReader = sourceReader + warningSuppression = + WarningSuppression.Resolver.FromCompilerOptions( + crackerResponse.ProjectOptions.OtherOptions, + sourceReader + ) ) // TODO: make it configurable if FableTransforms.transformFile is applied? @@ -221,6 +225,9 @@ module CodeServices = async { let fableLibDir = Path.getRelativePath currentFile crackerResponse.FableLibDir + // No `warningSuppression`: this overload is handed an already type-checked + // project rather than a `SourceReader`, so there is no source to scan for + // `// fable-disable` comments. Not a bug, just a limitation of the entry point. let compiler: Compiler = CompilerImpl( currentFile, @@ -306,6 +313,12 @@ module CodeServices = let opts = cliArgs.CompilerOptions + let warningSuppression = + WarningSuppression.Resolver.FromCompilerOptions( + crackerResponse.ProjectOptions.OtherOptions, + sourceReader + ) + let! compiledFiles = dependentFiles |> Array.filter (fun filePath -> not (filePath.EndsWith(".fsi", StringComparison.Ordinal))) @@ -321,7 +334,7 @@ module CodeServices = fableLibDir, crackerResponse.OutputType, ?outDir = cliArgs.OutDir, - sourceReader = sourceReader + warningSuppression = warningSuppression ) let outputPath = Path.ChangeExtension(currentFile, ".js") diff --git a/src/Fable.Transforms/Fable.Transforms.fsproj b/src/Fable.Transforms/Fable.Transforms.fsproj index d2a0d0331b..86cbef389d 100644 --- a/src/Fable.Transforms/Fable.Transforms.fsproj +++ b/src/Fable.Transforms/Fable.Transforms.fsproj @@ -5,8 +5,8 @@ - + diff --git a/src/Fable.Transforms/Global/WarningCodes.fs b/src/Fable.Transforms/Global/WarningCodes.fs index 002e5b9be2..6334eb3b6f 100644 --- a/src/Fable.Transforms/Global/WarningCodes.fs +++ b/src/Fable.Transforms/Global/WarningCodes.fs @@ -10,14 +10,47 @@ let private CultureInfoIgnored = "FABLE0001" [] let private StringSecondArgumentIgnored = "FABLE0002" +/// A `fable-disable*` directive named something that isn't in this registry - most likely a typo. +[] +let UnknownSuppressionCode = "FABLE0003" + +/// A `fable-disable*` directive that never suppressed anything, so it can be deleted. +[] +let UnusedSuppressionDirective = "FABLE0004" + +/// A bare `// fable-disable` block, which would silence every Fable warning up to end of file. +[] +let SuppressionBlockWithoutCode = "FABLE0005" + +/// Every code the compiler can emit. Directives naming anything else are reported as typos, so +/// a new code MUST be added here as well as given its own function below. +let knownCodes = + set + [ + CultureInfoIgnored + StringSecondArgumentIgnored + UnknownSuppressionCode + UnusedSuppressionDirective + SuppressionBlockWithoutCode + ] + /// `String.StartsWith`/`EndsWith` with a `CultureInfo` argument: the comparison always runs /// with the target's default culture rules, the argument is accepted but has no effect. /// Used in both the JS/TS and Python replacements. -let cultureInfoIgnored () = - CultureInfoIgnored, "CultureInfo argument is ignored" +let cultureInfoIgnored = CultureInfoIgnored, "CultureInfo argument is ignored" -/// `String.Contains`/`StartsWith`/`EndsWith` called with a `StringComparison` argument on the -/// Dart target: only the comparison itself is honored, `methodName` fills in which one so the -/// message stays specific (e.g. "String.Contains: second argument is ignored"). +/// `String.Contains`/`StartsWith`/`EndsWith` called with an extra argument on the Dart target +/// (a `StringComparison`, a `CultureInfo`, ...): only the comparison itself is honored. +/// `methodName` fills in which one so the message stays specific. let stringSecondArgumentIgnored (methodName: string) = - StringSecondArgumentIgnored, $"String.{methodName}: second argument is ignored" + StringSecondArgumentIgnored, $"String.%s{methodName}: second argument is ignored" + +let unknownSuppressionCode (code: string) = + UnknownSuppressionCode, $"Unknown warning code '%s{code}' in a fable-disable directive" + +let unusedSuppressionDirective = + UnusedSuppressionDirective, "This fable-disable directive doesn't suppress anything" + +let suppressionBlockWithoutCode = + SuppressionBlockWithoutCode, + "A 'fable-disable' block must list the warning codes it suppresses, otherwise it silences every Fable warning until the end of the file" diff --git a/src/Fable.Transforms/Global/WarningSuppression.fs b/src/Fable.Transforms/Global/WarningSuppression.fs index 8a4307e481..5aba7a5263 100644 --- a/src/Fable.Transforms/Global/WarningSuppression.fs +++ b/src/Fable.Transforms/Global/WarningSuppression.fs @@ -2,32 +2,82 @@ /// disable-line/next-line/block model), via real comment tokens - not raw text matching. module Fable.Transforms.WarningSuppression +open Fable +open System.Collections.Concurrent open System.Text.RegularExpressions open FSharp.Compiler.Tokenization -type private BlockState = - | NoneDisabled - | AllDisabledExcept of Set - | SpecificDisabled of Set +type private DirectiveKind = + | DisableLine + | DisableNextLine + | Disable + | Enable +[] type private Directive = - | DisableLine of codes: Set option * line: int - | DisableNextLine of codes: Set option * line: int - | Disable of codes: Set option * line: int - | Enable of codes: Set option * line: int + { + Kind: DirectiveKind + /// `None` means "every code" + Codes: Set option + Line: int + /// Set the first time this directive suppresses something. + mutable Used: bool + /// Set when the directive was already reported for something else (typo'd code, bare + /// block): no point telling the user it is unused on top of that. + mutable Reported: bool + } +type private BlockState = + | NoneDisabled + /// Blanket disable opened by this directive, minus the codes since re-enabled. + | AllDisabledExcept of opener: Directive * enabled: Set + /// code -> the directive that disabled it + | SpecificDisabled of Map + +/// A problem with a directive itself (typo'd code, directive that suppresses nothing, ...), +/// reported once the file it belongs to has finished compiling. +type DirectiveDiagnostic = + { + Code: string + Message: string + Line: int + } + +// The name must be followed by end-of-comment, whitespace or `:` so that `fable-disabled` or +// `fable-disable-lines` aren't mistaken for a bare directive. `:` is then optionally consumed let private directiveRegex = - Regex(@"^fable-(disable-next-line|disable-line|disable|enable)(?:\s+(.+))?$", RegexOptions.Compiled) + Regex(@"^fable-(disable-next-line|disable-line|disable|enable)(?=$|\s|:)\s*:?\s*(.*)$", RegexOptions.Compiled) + +let private codeSeparators = [| ' '; ','; '\t' |] + +/// Cheap pre-filter: tokenizing a source file to look for directives that aren't there is pure +/// waste, and the vast majority of files contain none. +let private mayContainDirectives (source: string) = + source.IndexOf("fable-disable", System.StringComparison.Ordinal) >= 0 + || source.IndexOf("fable-enable", System.StringComparison.Ordinal) >= 0 + +/// Splits a directive's argument text into codes, dropping the ESLint-style ` -- justification` +/// tail. Codes are upper-cased so `fable0001` matches, and anything not in the registry is +/// returned separately so the caller can report the typo instead of silently ignoring it. +let private parseCodes (raw: string) : Set option * string list = + let raw = + match raw.IndexOf("--", System.StringComparison.Ordinal) with + | -1 -> raw + | i -> raw.Substring(0, i) + + let tokens = + raw.Split(codeSeparators, System.StringSplitOptions.RemoveEmptyEntries) + |> Array.map (fun t -> t.ToUpperInvariant()) + + let known, unknown = tokens |> Array.partition WarningCodes.knownCodes.Contains -let private parseCodes (s: string) = let codes = - s.Split([| ' '; ','; '\t' |], System.StringSplitOptions.RemoveEmptyEntries) - |> Set.ofArray + if Array.isEmpty tokens then + None + else + Some(Set.ofArray known) - if Set.isEmpty codes then - None - else - Some codes + codes, List.ofArray unknown let private stripCommentMarkers (raw: string) = let raw = @@ -46,25 +96,34 @@ let private stripCommentMarkers (raw: string) = raw.Trim() -let private tryParseDirective (line: int) (commentText: string) : Directive option = +let private tryParseDirective (line: int) (commentText: string) : (Directive * string list) option = let text = stripCommentMarkers commentText let m = directiveRegex.Match(text) if not m.Success then None else - let codes = - if m.Groups[2].Success then - parseCodes m.Groups[2].Value - else - None - - match m.Groups[1].Value with - | "disable-line" -> Some(DisableLine(codes, line)) - | "disable-next-line" -> Some(DisableNextLine(codes, line)) - | "disable" -> Some(Disable(codes, line)) - | "enable" -> Some(Enable(codes, line)) - | _ -> None + let codes, unknown = parseCodes m.Groups[2].Value + + let kind = + match m.Groups[1].Value with + | "disable-line" -> Some DisableLine + | "disable-next-line" -> Some DisableNextLine + | "disable" -> Some Disable + | "enable" -> Some Enable + | _ -> None + + kind + |> Option.map (fun kind -> + { + Kind = kind + Codes = codes + Line = line + Used = false + Reported = false + }, + unknown + ) /// Gathers each line's comment token runs as plain text (a line can have more than one, e.g. a /// block comment then a trailing line comment); also returns the lexer state to carry into the @@ -104,128 +163,311 @@ let private scanLineComments let endState = loop initialState // Materialize each run's text; endState lets the caller resume correctly on the next line. - (runs |> Seq.map string |> List.ofSeq), endState + (runs |> Seq.map _.ToString() |> List.ofSeq), endState /// Computed, queryable suppression info for a single source file. type FileSuppressions = private { - /// 1-based line -> codes suppressed specifically on that line (None = all codes) - LineOnly: Map option> + /// Every parsed directive, in source order. + Directives: Directive[] + /// 1-based line -> the line-scoped directives landing on it + LineOnly: Map /// index (line - 1) -> block-disable state as of (and including) that line BlockAtLine: BlockState[] + /// Problems found while parsing (typo'd codes, bare block disables) + ParseDiagnostics: DirectiveDiagnostic list } - /// Is a diagnostic with the given code (None = no code assigned to it) suppressed on this line? - member this.IsSuppressed(line: int, code: string option) = - let lineSuppressed = + /// Is a diagnostic with the given code (None = no code assigned to it) suppressed anywhere in + /// `[startLine, endLine]`? Line-scoped directives match on any line of the range, so a + /// trailing `// fable-disable-line` still works on a multi-line expression. The block state is + /// read at `startLine` only: a `fable-disable` buried inside a large expression shouldn't + /// retroactively silence a warning anchored above it. + member this.IsSuppressed(startLine: int, endLine: int, code: string option) = + let matches (d: Directive) = + match d.Codes with + | None -> true + | Some codes -> code |> Option.map codes.Contains |> Option.defaultValue false + + let mutable suppressed = false + + for line in startLine .. max startLine endLine do match Map.tryFind line this.LineOnly with - | Some None -> true - | Some(Some codes) -> code |> Option.map codes.Contains |> Option.defaultValue false + | Some directives -> + for d in directives do + if matches d then + d.Used <- true + suppressed <- true + | None -> () + + if not suppressed && startLine >= 1 && startLine <= this.BlockAtLine.Length then + match this.BlockAtLine[startLine - 1] with + | NoneDisabled -> () + | AllDisabledExcept(opener, enabled) -> + let hit = + match code with + | None -> true + | Some c -> not (Set.contains c enabled) + + if hit then + opener.Used <- true + suppressed <- true + | SpecificDisabled disabled -> + match code |> Option.bind (fun c -> Map.tryFind c disabled) with + | Some opener -> + opener.Used <- true + suppressed <- true + | None -> () + + suppressed + + member this.IsSuppressed(line: int, code: string option) = this.IsSuppressed(line, line, code) + + /// Like `IsSuppressed`, but only honours directives that name the code explicitly. Reserved + /// for the diagnostics about directives themselves: a bare `// fable-disable` must not be + /// able to silence the warning that exists to tell you a bare `// fable-disable` is dangerous. + member this.IsExplicitlySuppressed(line: int, code: string) = + let named (d: Directive) = + match d.Codes with | None -> false + | Some codes -> codes.Contains code + + let lineSuppressed = + match Map.tryFind line this.LineOnly |> Option.defaultValue [] |> List.filter named with + | [] -> false + | matched -> + for d in matched do + d.Used <- true + + true let blockSuppressed = if line < 1 || line > this.BlockAtLine.Length then false else match this.BlockAtLine[line - 1] with - | NoneDisabled -> false - | AllDisabledExcept enabled -> - match code with - | None -> true - | Some c -> not (Set.contains c enabled) + | NoneDisabled + | AllDisabledExcept _ -> false | SpecificDisabled disabled -> - match code with + match Map.tryFind code disabled with + | Some opener -> + opener.Used <- true + true | None -> false - | Some c -> Set.contains c disabled lineSuppressed || blockSuppressed + /// Parse problems plus every directive that never suppressed anything. Only meaningful once + /// the whole compilation is over: an inlined call can suppress through a directive in a file + /// other than the one currently being compiled. + member this.GetDiagnostics() = + let unused = + this.Directives + |> Array.choose (fun d -> + // `fable-enable` re-opens warnings rather than suppressing them, so "unused" has + // no meaning for it - ESLint doesn't report those either. A directive whose codes + // are already reported as typos is obviously unused too; one message is enough. + if d.Kind = Enable || d.Used || d.Reported then + None + else + let code, message = WarningCodes.unusedSuppressionDirective + + Some + { + Code = code + Message = message + Line = d.Line + } + ) + |> List.ofArray + + this.ParseDiagnostics @ unused + static member Empty = { + Directives = [||] LineOnly = Map.empty BlockAtLine = [||] + ParseDiagnostics = [] } -let private transition (state: BlockState) (codes: Set option) (isDisable: bool) = - match isDisable, codes, state with - | true, None, _ -> AllDisabledExcept Set.empty - | true, Some codes, NoneDisabled -> SpecificDisabled codes - | true, Some codes, SpecificDisabled s -> SpecificDisabled(Set.union s codes) - | true, Some codes, AllDisabledExcept ex -> AllDisabledExcept(Set.difference ex codes) +/// Folds one `fable-disable`/`fable-enable` into the running block state. The directive itself is +/// carried through so that whatever it ends up suppressing can mark it used. +let private transition (state: BlockState) (d: Directive) = + match d.Kind = Disable, d.Codes, state with + | true, None, _ -> AllDisabledExcept(d, Set.empty) + | true, Some codes, NoneDisabled -> SpecificDisabled(codes |> Seq.map (fun c -> c, d) |> Map.ofSeq) + | true, Some codes, SpecificDisabled m -> SpecificDisabled((m, codes) ||> Seq.fold (fun m c -> Map.add c d m)) + | true, Some codes, AllDisabledExcept(opener, ex) -> AllDisabledExcept(opener, Set.difference ex codes) | false, None, _ -> NoneDisabled | false, Some _, NoneDisabled -> NoneDisabled - | false, Some codes, SpecificDisabled s -> SpecificDisabled(Set.difference s codes) - | false, Some codes, AllDisabledExcept ex -> AllDisabledExcept(Set.union ex codes) + | false, Some codes, SpecificDisabled m -> SpecificDisabled((m, codes) ||> Seq.fold (fun m c -> Map.remove c m)) + | false, Some codes, AllDisabledExcept(opener, ex) -> AllDisabledExcept(opener, Set.union ex codes) -/// Scans the given source text for `fable-disable*`/`fable-enable*` comments and builds a -/// queryable `FileSuppressions` snapshot. Meant to be computed once per file and cached. -let compute (source: string) : FileSuppressions = +let private computeDirectives (defines: string list) (source: string) = let lines = source.Replace("\r\n", "\n").Split('\n') - - if lines.Length = 0 then - FileSuppressions.Empty - else - let sourceTok = FSharpSourceTokenizer([], None, None, None) - let directives = ResizeArray() - let mutable state = FSharpTokenizerLexState.Initial - - for i in 0 .. lines.Length - 1 do - let lineNo = i + 1 - let tokenizer = sourceTok.CreateLineTokenizer(lines[i]) - let runs, newState = scanLineComments tokenizer state lines[i] - state <- newState - - for run in runs do - tryParseDirective lineNo run |> Option.iter directives.Add - - let lineOnly = - (Map.empty, directives) - ||> Seq.fold (fun acc d -> - let merge line (codes: Set option) = - let merged = - match Map.tryFind line acc, codes with - | Some None, _ - | _, None -> None - | None, Some c -> Some c - | Some(Some existing), Some c -> Some(Set.union existing c) - - Map.add line merged acc - - match d with - | DisableLine(codes, line) -> merge line codes - | DisableNextLine(codes, line) -> merge (line + 1) codes - | Disable _ - | Enable _ -> acc - ) - - let blockDirectivesByLine = - directives - |> Seq.choose ( - function - | Disable(codes, line) -> Some(line, codes, true) - | Enable(codes, line) -> Some(line, codes, false) - | DisableLine _ - | DisableNextLine _ -> None - ) - |> Seq.groupBy (fun (line, _, _) -> line) - |> Map.ofSeq - - // Filling this with hundreds/thousands of NoneDisabled is nearly free: F# compiles a - // nullary DU case to a singleton, so every slot is a pointer to the same object. - let blockAtLine = Array.create lines.Length NoneDisabled - let mutable current = NoneDisabled - - for lineNo in 1 .. lines.Length do - match Map.tryFind lineNo blockDirectivesByLine with - | Some ds -> - for _, codes, isDisable in ds do - current <- transition current codes isDisable + let sourceTok = FSharpSourceTokenizer(defines, None, None, None) + let directives = ResizeArray() + let parseDiagnostics = ResizeArray() + let mutable state = FSharpTokenizerLexState.Initial + + for i in 0 .. lines.Length - 1 do + let lineNo = i + 1 + let tokenizer = sourceTok.CreateLineTokenizer(lines[i]) + let runs, newState = scanLineComments tokenizer state lines[i] + state <- newState + + for run in runs do + match tryParseDirective lineNo run with | None -> () + | Some(directive, unknownCodes) -> + let flag (code, message) = + directive.Reported <- true + + parseDiagnostics.Add + { + Code = code + Message = message + Line = lineNo + } + + for unknown in unknownCodes do + flag (WarningCodes.unknownSuppressionCode unknown) + + // A bare `fable-disable` block silences every Fable warning to end of file, which + // is almost never what people mean - flag it, but honour it. + if directive.Kind = Disable && Option.isNone directive.Codes then + flag WarningCodes.suppressionBlockWithoutCode + + directives.Add directive + + let lineOnly = + (Map.empty, directives) + ||> Seq.fold (fun acc d -> + let add line = + let existing = Map.tryFind line acc |> Option.defaultValue [] + Map.add line (d :: existing) acc + + match d.Kind with + | DisableLine -> add d.Line + | DisableNextLine -> add (d.Line + 1) + | Disable + | Enable -> acc + ) + + let blockDirectivesByLine = + directives + |> Seq.filter (fun d -> + match d.Kind with + | Disable + | Enable -> true + | DisableLine + | DisableNextLine -> false + ) + |> Seq.groupBy (fun d -> d.Line) + |> Seq.map (fun (line, ds) -> line, List.ofSeq ds) + |> Map.ofSeq + + // Filling this with hundreds/thousands of NoneDisabled is nearly free: F# compiles a + // nullary DU case to a singleton, so every slot is a pointer to the same object. + let blockAtLine = Array.create lines.Length NoneDisabled + let mutable current = NoneDisabled + + for lineNo in 1 .. lines.Length do + match Map.tryFind lineNo blockDirectivesByLine with + | Some ds -> + for d in ds do + current <- transition current d + | None -> () + + blockAtLine[lineNo - 1] <- current + + { + Directives = Seq.toArray directives + LineOnly = lineOnly + BlockAtLine = blockAtLine + ParseDiagnostics = List.ofSeq parseDiagnostics + } - blockAtLine[lineNo - 1] <- current +/// Scans the given source text for `fable-disable*`/`fable-enable*` comments and builds a +/// queryable `FileSuppressions` snapshot. Meant to be computed once per file and cached. +/// +/// `defines` must be the same conditional-compilation symbols FCS was given (`FABLE_COMPILER`, +/// the project's `DefineConstants`, ...). Without them the tokenizer reports everything inside +/// an `#if` block as inactive code rather than comments, so directives sitting next to the very +/// warnings they're meant to suppress would be invisible. +let compute (defines: string list) (source: string) : FileSuppressions = + if mayContainDirectives source then + computeDirectives defines source + else + FileSuppressions.Empty - { - LineOnly = lineOnly - BlockAtLine = blockAtLine - } +/// Suppression snapshots shared by every file of a project: computing them is per-file work, but +/// warnings can point at any file (inlined calls) and files compile in parallel. Keyed by the +/// content hash `SourceReader` already returns, so a file edited under watch mode recomputes +/// instead of being scanned against stale text. +type private Cache() = + let entries = ConcurrentDictionary() + + /// May throw `KeyNotFoundException` for a file outside the project (e.g. the source of a + /// precompiled dll) - that's the caller's cue that there's nothing to suppress with. + member _.GetOrCompute(fileName: string, defines: string list, read: SourceReader) = + let hash, source = read fileName + + // GetOrAdd rather than a TryGetValue/assign pair: two threads racing on the same file + // would otherwise end up with separate snapshots, splitting the "directive was used" + // flags between them and producing bogus unused-directive reports. + let cachedHash, suppressions = + entries.GetOrAdd(fileName, fun _ -> hash, compute defines source.Value) + + if cachedHash = hash then + suppressions + else + // The file changed under us, so the snapshot is against the wrong text. + let fresh = hash, compute defines source.Value + entries[fileName] <- fresh + snd fresh + +/// Extracts the conditional-compilation symbols out of the option list FCS is given +/// (`FSharpProjectOptions.OtherOptions`), which is the only place the project's `DefineConstants` +/// and Fable's own `FABLE_COMPILER*` symbols are both present. +let definesFromCompilerOptions (otherOptions: string seq) = + otherOptions + |> Seq.choose (fun opt -> + if opt.StartsWith("--define:", System.StringComparison.Ordinal) then + Some(opt.Substring("--define:".Length)) + else + None + ) + |> List.ofSeq + +/// Project-wide entry point: resolves a file name to its suppressions, computing and caching on +/// first use. One instance per compiled project - it is shared across the per-file compilers. +type Resolver(defines: string list, read: SourceReader) = + let cache = Cache() + + static member FromCompilerOptions(otherOptions: string seq, read: SourceReader) = + Resolver(definesFromCompilerOptions otherOptions, read) + + member _.For(fileName: string) = + try + cache.GetOrCompute(fileName, defines, read) + with :? System.Collections.Generic.KeyNotFoundException -> + // Not a file of this project (e.g. the source of a precompiled dll): nothing to read, + // so nothing can be suppressed. Any other failure is a real problem and must surface. + FileSuppressions.Empty + + /// The directive problems of the given files, minus the ones a directive silences in turn. + /// Call once the whole compilation is over: a warning raised while compiling one file can be + /// suppressed by a directive living in another. + member this.GetDiagnostics(fileNames: string seq) = + fileNames + |> Seq.collect (fun fileName -> + let suppressions = this.For(fileName) + + suppressions.GetDiagnostics() + |> List.filter (fun d -> not (suppressions.IsExplicitlySuppressed(d.Line, d.Code))) + |> List.map (fun d -> fileName, d) + ) + |> List.ofSeq diff --git a/src/Fable.Transforms/Python/Replacements.fs b/src/Fable.Transforms/Python/Replacements.fs index 8e3d24738c..225b612a1d 100644 --- a/src/Fable.Transforms/Python/Replacements.fs +++ b/src/Fable.Transforms/Python/Replacements.fs @@ -1512,7 +1512,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt Helper.LibCall(com, "string", "starts_with", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) |> Some | "StartsWith", Some c, [ value; ignoreCase; _culture ] -> - WarningCodes.cultureInfoIgnored () |> addWarningWithCode com ctx.InlinePath r + WarningCodes.cultureInfoIgnored |> addWarningWithCode com ctx.InlinePath r let args = [ value; ignoreCase ] Helper.LibCall(com, "string", "starts_with", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) @@ -1524,7 +1524,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt Helper.LibCall(com, "string", "ends_with", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) |> Some | "EndsWith", Some c, [ value; ignoreCase; _culture ] -> - WarningCodes.cultureInfoIgnored () |> addWarningWithCode com ctx.InlinePath r + WarningCodes.cultureInfoIgnored |> addWarningWithCode com ctx.InlinePath r let args = [ value; ignoreCase ] Helper.LibCall(com, "string", "ends_with", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) diff --git a/src/Fable.Transforms/Replacements.fs b/src/Fable.Transforms/Replacements.fs index 7b12349c53..bdf003bc3f 100644 --- a/src/Fable.Transforms/Replacements.fs +++ b/src/Fable.Transforms/Replacements.fs @@ -1723,7 +1723,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt Helper.LibCall(com, "String", "startsWith", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) |> Some | "StartsWith", Some c, [ value; ignoreCase; _culture ] -> - WarningCodes.cultureInfoIgnored () |> addWarningWithCode com ctx.InlinePath r + WarningCodes.cultureInfoIgnored |> addWarningWithCode com ctx.InlinePath r let args = [ value; ignoreCase ] Helper.LibCall(com, "String", "startsWith", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) @@ -1733,7 +1733,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt Helper.LibCall(com, "String", "endsWith", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) |> Some | "EndsWith", Some c, [ value; ignoreCase; _culture ] -> - WarningCodes.cultureInfoIgnored () |> addWarningWithCode com ctx.InlinePath r + WarningCodes.cultureInfoIgnored |> addWarningWithCode com ctx.InlinePath r let args = [ value; ignoreCase ] Helper.LibCall(com, "String", "endsWith", t, args, i.SignatureArgTypes, thisArg = c, ?loc = r) diff --git a/src/Fable.Transforms/State.fs b/src/Fable.Transforms/State.fs index 448d9ca3e8..13372eaa4c 100644 --- a/src/Fable.Transforms/State.fs +++ b/src/Fable.Transforms/State.fs @@ -286,7 +286,7 @@ type CompilerImpl ?watchDependencies: HashSet, ?logs: ResizeArray, ?isPrecompilingInlineFunction: bool, - ?sourceReader: SourceReader + ?warningSuppression: WarningSuppression.Resolver ) = @@ -294,23 +294,11 @@ type CompilerImpl let outType = defaultArg outType OutputType.Exe let logs = Option.defaultWith ResizeArray logs let fableLibraryDir = fableLibDir.TrimEnd('/') - let suppressionsCache = Dictionary() let getSuppressions fileName = - match suppressionsCache.TryGetValue(fileName) with - | true, s -> s - | false, _ -> - let suppressions = - match sourceReader with - | None -> WarningSuppression.FileSuppressions.Empty - | Some read -> - try - (snd (read fileName)).Value |> WarningSuppression.compute - with _ -> - WarningSuppression.FileSuppressions.Empty - - suppressionsCache[fileName] <- suppressions - suppressions + match warningSuppression with + | Some resolver -> resolver.For(fileName) + | None -> WarningSuppression.FileSuppressions.Empty member _.Logs = logs.ToArray() @@ -354,7 +342,7 @@ type CompilerImpl ?watchDependencies = watchDependencies, logs = logs, isPrecompilingInlineFunction = true, - ?sourceReader = sourceReader + ?warningSuppression = warningSuppression ) member _.GetImplementationFile(fileName) = @@ -424,7 +412,9 @@ type CompilerImpl | Some f -> f | None -> currentFile - (getSuppressions file).IsSuppressed(r.start.line, code) + // The whole range, not just its first line, so a trailing + // `// fable-disable-line` still works on a multi-line expression. + (getSuppressions file).IsSuppressed(r.start.line, r.``end``.line, code) ) if not isSuppressed then diff --git a/src/Fable.Transforms/Transforms.Util.fs b/src/Fable.Transforms/Transforms.Util.fs index 9d796cdede..a4263ef996 100644 --- a/src/Fable.Transforms/Transforms.Util.fs +++ b/src/Fable.Transforms/Transforms.Util.fs @@ -703,6 +703,8 @@ module Log = /// Same as `addWarning`, but tags the warning with a stable code (e.g. "FABLE0001") so it /// can be suppressed via `// fable-disable-line/-next-line/-enable CODE` comments. /// Meant to be used as `WarningCodes.someWarning arg1 arg2 |> addWarningWithCode com inlinePath range`. + /// + /// Note that a warning coming out of an inlined function is attributed to the file that *defines* it let addWarningWithCode (com: Compiler) inlinePath range ((code, warning): string * string) = addLogWithCode com inlinePath range warning Severity.Warning (Some code) diff --git a/src/fable-standalone/src/Fable.Standalone.fsproj b/src/fable-standalone/src/Fable.Standalone.fsproj index f6bc334cd9..6daecdaa9c 100644 --- a/src/fable-standalone/src/Fable.Standalone.fsproj +++ b/src/fable-standalone/src/Fable.Standalone.fsproj @@ -22,8 +22,8 @@ - + diff --git a/src/fable-standalone/src/Main.fs b/src/fable-standalone/src/Main.fs index 1504c64b40..8c0043de10 100644 --- a/src/fable-standalone/src/Main.fs +++ b/src/fable-standalone/src/Main.fs @@ -152,6 +152,7 @@ let makeCompiler fableLibrary typedArrays language fsharpOptions project fileNam ?typedArrays = typedArrays ) + // No `warningSuppression`: the REPL has no `SourceReader` to scan, so `// fable-disable` comments are a no-op here. CompilerImpl(fileName, project, options, fableLibrary) let makeProject (projectOptions: FSharpProjectOptions) (checkResults: FSharpCheckProjectResults) = diff --git a/tests/Integration/Compiler/CompilerMessagesTests.fs b/tests/Integration/Compiler/CompilerMessagesTests.fs index 402bd7f674..5f78cfa4d4 100644 --- a/tests/Integration/Compiler/CompilerMessagesTests.fs +++ b/tests/Integration/Compiler/CompilerMessagesTests.fs @@ -137,10 +137,9 @@ open System.Globalization |> ignore testCase "The same code covers both StartsWith and EndsWith call sites" <| fun _ -> - // Regression test for a real bug: StartsWith and EndsWith both raise the same logical - // "CultureInfo argument is ignored" warning from two separate call sites in - // Replacements.fs, sharing WarningCodes.CultureInfoIgnored. Both must be suppressible - // by one code, and (in Python's Replacements.fs) this used to have no code at all. + // StartsWith and EndsWith raise the same logical "CultureInfo argument is ignored" warning + // from two separate call sites in Replacements.fs, sharing WarningCodes.CultureInfoIgnored. + // One code must suppress both, otherwise the registry has failed at its only job. let source = """ open System.Globalization @@ -216,4 +215,213 @@ let s = "// fable-disable-line FABLE0001" compile source |> Assert.Exists.warningWith "CultureInfo argument is ignored" |> ignore + + testCase "A directive inside a #if FABLE_COMPILER block is honoured" <| fun _ -> + let source = + """ +open System.Globalization +#if FABLE_COMPILER +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 +#endif +""" + compile source + |> Assert.Code.noWarning "FABLE0001" + |> ignore + + testCase "A warning inside a #if FABLE_COMPILER block still fires without a directive" <| fun _ -> + // Guards the test above from passing vacuously because the block was compiled out. + let source = + """ +open System.Globalization +#if FABLE_COMPILER +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +#endif +""" + compile source + |> Assert.Code.warning "FABLE0001" + |> ignore + + testCase "The formatted output carries the warning code" <| fun _ -> + // Without this there is no way to discover which code to put in a directive. + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + let formatted = + compile source + |> List.filter (fun log -> log.Code = Some "FABLE0001") + |> List.map (Fable.Cli.Main.Util.formatLog Compiler.Cached.projDir) + + match formatted with + | [] -> failwith "Expected a FABLE0001 warning" + | messages -> equal true (messages |> List.forall (fun m -> m.Contains "warning FABLE FABLE0001:")) + + testCase "Errors are never suppressed, not even by a blanket fable-disable" <| fun _ -> + let source = + """ +open Fable.Core.JsInterop + +type Response = + abstract fn: int -> int + abstract prop: bool with get, set + +// fable-disable +let res = jsOptions (fun o -> o.fn <- (fun i -> i)) +""" + compile source + |> Assert.Exists.errorWith "Cannot set a non-property member in 'jsOptions'" + |> ignore + + testCase "A directive written as a block comment is honoured" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore (* fable-disable-line FABLE0001 *) +""" + compile source + |> Assert.Code.noWarning "FABLE0001" + |> ignore + + testCase "A trailing directive suppresses a warning spanning several lines" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith( + "a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 +""" + compile source + |> Assert.Code.noWarning "FABLE0001" + |> ignore + + testCase "A fable-disable block with no fable-enable runs to the end of the file" <| fun _ -> + let source = + """ +open System.Globalization +// fable-disable FABLE0001 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Code.noWarning "FABLE0001" + |> ignore + + testCase "A colon separator and a lower-case code are accepted" <| fun _ -> + // What people coming from `# noqa: E501` and `@ts-ignore` will write. + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line: fable0001 +""" + compile source + |> Assert.Code.noWarning "FABLE0001" + |> ignore + + testCase "A justification after -- is not parsed as codes" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 -- culture is irrelevant here +""" + compile source + |> Assert.Code.noWarning "FABLE0001" + |> Assert.Code.noWarning "FABLE0003" + |> ignore + + testCase "A typo'd code is reported instead of silently suppressing nothing" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABEL0001 +""" + compile source + |> Assert.Code.warning "FABLE0003" + |> Assert.Code.warning "FABLE0001" + // The typo report is the actionable one; don't pile "and it's unused" on top of it. + |> Assert.Code.noWarning "FABLE0004" + |> ignore + + testCase "A directive that suppresses nothing is reported as unused" <| fun _ -> + let source = + """ +open System.Globalization +// fable-disable-next-line FABLE0001 +let answer = 42 +""" + compile source + |> Assert.Code.warning "FABLE0004" + |> ignore + + testCase "A directive that does its job is not reported as unused" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 +""" + compile source + |> Assert.Code.noWarning "FABLE0004" + |> ignore + + testCase "A fable-disable block with no codes is reported" <| fun _ -> + // It would otherwise silence every Fable warning to the end of the file, unnoticed. + let source = + """ +open System.Globalization +// fable-disable +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0005" + |> Assert.Code.noWarning "FABLE0001" + |> ignore + + testCase "A word merely starting with a directive name is not a directive" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disabled for now +""" + compile source + |> Assert.Code.warning "FABLE0001" + |> ignore + + testCase "Directives are found in CRLF sources" <| fun _ -> + let source = + [ "" + "open System.Globalization" + "\"abc\".StartsWith(\"a\", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001" + "" ] + |> String.concat "\r\n" + + compile source + |> Assert.Code.noWarning "FABLE0001" + |> ignore + + testCase "A warning from an inlined function is suppressed at its definition" <| fun _ -> + // The warning is attributed to the file the inline function is *defined* in, so that is + // where the directive has to go - the call site can't suppress it. + let source = + """ +open System.Globalization +let inline startsWithCulture (s: string) = + s.StartsWith("a", true, CultureInfo.InvariantCulture) // fable-disable-line FABLE0001 + +startsWithCulture "abc" |> ignore +""" + compile source + |> Assert.Code.noWarning "FABLE0001" + |> ignore + + testCase "A directive at the call site does not suppress an inlined function's warning" <| fun _ -> + let source = + """ +open System.Globalization +let inline startsWithCulture (s: string) = + s.StartsWith("a", true, CultureInfo.InvariantCulture) + +startsWithCulture "abc" |> ignore // fable-disable-line FABLE0001 +""" + compile source + |> Assert.Code.warning "FABLE0001" + |> ignore ] diff --git a/tests/Integration/Compiler/Util/Compiler.fs b/tests/Integration/Compiler/Util/Compiler.fs index 4a6b62ffa7..d42fafcdb0 100644 --- a/tests/Integration/Compiler/Util/Compiler.fs +++ b/tests/Integration/Compiler/Util/Compiler.fs @@ -32,7 +32,9 @@ module Compiler = let sourceFile = IO.Path.Join(projDir, "Program.fs" ) |> Path.normalizeFullPath let cliArgs = - let compilerOptions = CompilerOptionsHelper.Make() + // The real CLI always defines these (Entry.fs), and code guarded by them is exactly where + // Fable warnings - and the comments suppressing them - live. + let compilerOptions = CompilerOptionsHelper.Make(define = ["FABLE_COMPILER"; "FABLE_COMPILER_JAVASCRIPT"]) { CliArgs.ProjectFile = projFile FableLibraryPath = None RootDir = projDir @@ -141,6 +143,8 @@ module Compiler = let errorOrWarning: Result -> bool = List.isEmpty >> not let matching = List.exists let withMsg (txt: string) = List.exists (fun m -> m.Message.Contains txt) + module Code = + let is (code: string) (msg: LogEntry) = msg.Code = Some code module Text = let contains (txt: string) msg = msg.Message.Contains(txt, StringComparison.InvariantCultureIgnoreCase) let isMatch (regex: System.Text.RegularExpressions.Regex) (msg: LogEntry) = @@ -205,6 +209,16 @@ module Compiler = Test.All.warning |> orFail expected.All.Warning + /// Assertions on the stable `FABLExxxx` codes rather than on message text: what a + /// `// fable-disable` comment targets is the code, so that's what the tests must pin. + module Code = + let warning (code: string) = + Test.Exists.matching (Test.Is.warning >&> Test.Code.is code) + |> orFail (expected.Warning.With code) + let noWarning (code: string) = + (not << Test.Exists.matching (Test.Is.warning >&> Test.Code.is code)) + |> orFail (expected.No.Warning.With code) + module Exists = let errorWith (txt: string) = Test.Exists.matching (Test.Is.error >&> Test.Text.contains txt) From 2996a5b5d89c969024a17f830075eb0ec0fb4853 Mon Sep 17 00:00:00 2001 From: Mangel Maxime Date: Thu, 13 Aug 2026 17:30:22 +0200 Subject: [PATCH 3/7] fix: make some of the warning non suppressible and reserve a range for Fable own usage --- src/Fable.Transforms/Global/WarningCodes.fs | 74 ++++++++----- .../Global/WarningSuppression.fs | 55 ++-------- src/Fable.Transforms/State.fs | 4 +- .../Compiler/CompilerHelpersTests.fs | 13 +++ .../Compiler/CompilerMessagesTests.fs | 101 +++++++++++------- 5 files changed, 136 insertions(+), 111 deletions(-) diff --git a/src/Fable.Transforms/Global/WarningCodes.fs b/src/Fable.Transforms/Global/WarningCodes.fs index 6334eb3b6f..f21834e5ce 100644 --- a/src/Fable.Transforms/Global/WarningCodes.fs +++ b/src/Fable.Transforms/Global/WarningCodes.fs @@ -1,49 +1,60 @@ -/// Central registry of stable codes + messages for `addWarningWithCode`: one function per -/// warning, so call sites sharing the same warning (e.g. StartsWith/EndsWith, JS/Python) can't -/// drift into different codes or wording. Codes are never reused/renumbered once published. -/// Usage: `WarningCodes.someWarning arg1 arg2 |> addWarningWithCode com inlinePath range`. +(* + Central registry for Fable warnings codes + + - `FABLE0001`-`FABLE0099` describe Fable's own suppression directivers. They are never suppressible. + - `FABLE0100` and up describe code from compilation result. Those can be suppressed +*) module Fable.Transforms.WarningCodes -[] -let private CultureInfoIgnored = "FABLE0001" - -[] -let private StringSecondArgumentIgnored = "FABLE0002" +(* + FABLE0001-0099: about Fable's own directives. Never suppressible. +*) /// A `fable-disable*` directive named something that isn't in this registry - most likely a typo. [] -let UnknownSuppressionCode = "FABLE0003" +let UnknownSuppressionCode = "FABLE0001" /// A `fable-disable*` directive that never suppressed anything, so it can be deleted. [] -let UnusedSuppressionDirective = "FABLE0004" +let UnusedSuppressionDirective = "FABLE0002" /// A bare `// fable-disable` block, which would silence every Fable warning up to end of file. [] -let SuppressionBlockWithoutCode = "FABLE0005" +let SuppressionBlockWithoutCode = "FABLE0003" + +(* + FABLE0100 and up: about the compiled code. Suppressible. +*) + +[] +let private CultureInfoIgnored = "FABLE0100" + +[] +let private StringSecondArgumentIgnored = "FABLE0101" -/// Every code the compiler can emit. Directives naming anything else are reported as typos, so -/// a new code MUST be added here as well as given its own function below. +/// Every code the compiler can emit, both bands. A directive naming anything else is reported as +/// a typo, so a new code MUST be added here as well as given its own function below. let knownCodes = set [ - CultureInfoIgnored - StringSecondArgumentIgnored UnknownSuppressionCode UnusedSuppressionDirective SuppressionBlockWithoutCode + CultureInfoIgnored + StringSecondArgumentIgnored ] -/// `String.StartsWith`/`EndsWith` with a `CultureInfo` argument: the comparison always runs -/// with the target's default culture rules, the argument is accepted but has no effect. -/// Used in both the JS/TS and Python replacements. -let cultureInfoIgnored = CultureInfoIgnored, "CultureInfo argument is ignored" - -/// `String.Contains`/`StartsWith`/`EndsWith` called with an extra argument on the Dart target -/// (a `StringComparison`, a `CultureInfo`, ...): only the comparison itself is honored. -/// `methodName` fills in which one so the message stays specific. -let stringSecondArgumentIgnored (methodName: string) = - StringSecondArgumentIgnored, $"String.%s{methodName}: second argument is ignored" +/// Can a `// fable-disable` comment silence a warning carrying this code? Codes below +/// `FABLE0100` describe the directives themselves and never can - see the band rules above. +/// This is what makes the banding an invariant rather than a naming convention: it holds +/// wherever a diagnostic is raised from, not just on the path that happens to raise them today. +/// +/// `None` is a warning with no code at all, which a bare `// fable-disable-line` still catches. +let isSuppressible (code: string option) = + match code with + | None -> true + // Every code is `FABLE` plus four digits, so ordinal comparison orders them numerically. + | Some code -> System.String.CompareOrdinal(code, "FABLE0100") >= 0 let unknownSuppressionCode (code: string) = UnknownSuppressionCode, $"Unknown warning code '%s{code}' in a fable-disable directive" @@ -54,3 +65,14 @@ let unusedSuppressionDirective = let suppressionBlockWithoutCode = SuppressionBlockWithoutCode, "A 'fable-disable' block must list the warning codes it suppresses, otherwise it silences every Fable warning until the end of the file" + +/// `String.StartsWith`/`EndsWith` with a `CultureInfo` argument: the comparison always runs +/// with the target's default culture rules, the argument is accepted but has no effect. +/// Used in both the JS/TS and Python replacements. +let cultureInfoIgnored = CultureInfoIgnored, "CultureInfo argument is ignored" + +/// `String.Contains`/`StartsWith`/`EndsWith` called with an extra argument on the Dart target +/// (a `StringComparison`, a `CultureInfo`, ...): only the comparison itself is honored. +/// `methodName` fills in which one so the message stays specific. +let stringSecondArgumentIgnored (methodName: string) = + StringSecondArgumentIgnored, $"String.%s{methodName}: second argument is ignored" diff --git a/src/Fable.Transforms/Global/WarningSuppression.fs b/src/Fable.Transforms/Global/WarningSuppression.fs index 5aba7a5263..d6f3c2b960 100644 --- a/src/Fable.Transforms/Global/WarningSuppression.fs +++ b/src/Fable.Transforms/Global/WarningSuppression.fs @@ -1,5 +1,7 @@ -/// Computes which diagnostics `// fable-disable/-enable...` comments suppress (ESLint's -/// disable-line/next-line/block model), via real comment tokens - not raw text matching. +(* + Computes which diagnostics `// fable-disable/-enable...` comments suppress (ESLint's + disable-line/next-line/block model), via real comment tokens - not raw text matching. +*) module Fable.Transforms.WarningSuppression open Fable @@ -224,40 +226,6 @@ type FileSuppressions = member this.IsSuppressed(line: int, code: string option) = this.IsSuppressed(line, line, code) - /// Like `IsSuppressed`, but only honours directives that name the code explicitly. Reserved - /// for the diagnostics about directives themselves: a bare `// fable-disable` must not be - /// able to silence the warning that exists to tell you a bare `// fable-disable` is dangerous. - member this.IsExplicitlySuppressed(line: int, code: string) = - let named (d: Directive) = - match d.Codes with - | None -> false - | Some codes -> codes.Contains code - - let lineSuppressed = - match Map.tryFind line this.LineOnly |> Option.defaultValue [] |> List.filter named with - | [] -> false - | matched -> - for d in matched do - d.Used <- true - - true - - let blockSuppressed = - if line < 1 || line > this.BlockAtLine.Length then - false - else - match this.BlockAtLine[line - 1] with - | NoneDisabled - | AllDisabledExcept _ -> false - | SpecificDisabled disabled -> - match Map.tryFind code disabled with - | Some opener -> - opener.Used <- true - true - | None -> false - - lineSuppressed || blockSuppressed - /// Parse problems plus every directive that never suppressed anything. Only meaningful once /// the whole compilation is over: an inlined call can suppress through a directive in a file /// other than the one currently being compiled. @@ -458,16 +426,11 @@ type Resolver(defines: string list, read: SourceReader) = // so nothing can be suppressed. Any other failure is a real problem and must surface. FileSuppressions.Empty - /// The directive problems of the given files, minus the ones a directive silences in turn. - /// Call once the whole compilation is over: a warning raised while compiling one file can be - /// suppressed by a directive living in another. + /// The directive problems of the given files. These are `FABLE0001`-band codes and so are + /// never themselves suppressible - they flag comment text that is wrong to keep, so the only + /// correct answer is to edit it. Call once the whole compilation is over: a warning raised + /// while compiling one file can be suppressed by a directive living in another. member this.GetDiagnostics(fileNames: string seq) = fileNames - |> Seq.collect (fun fileName -> - let suppressions = this.For(fileName) - - suppressions.GetDiagnostics() - |> List.filter (fun d -> not (suppressions.IsExplicitlySuppressed(d.Line, d.Code))) - |> List.map (fun d -> fileName, d) - ) + |> Seq.collect (fun fileName -> this.For(fileName).GetDiagnostics() |> List.map (fun d -> fileName, d)) |> List.ofSeq diff --git a/src/Fable.Transforms/State.fs b/src/Fable.Transforms/State.fs index 13372eaa4c..0cd2923ece 100644 --- a/src/Fable.Transforms/State.fs +++ b/src/Fable.Transforms/State.fs @@ -397,9 +397,11 @@ type CompilerImpl | _ -> () member _.AddLog(msg, severity, ?range, ?fileName: string, ?tag: string, ?code: string) = - // Only warnings can be suppressed, errors always surface (matches F#'s own #nowarn) + // Only warnings can be suppressed, errors always surface (matches F#'s own #nowarn), + // and neither can the warnings about the `fable-disable` directives themselves. let isSuppressed = severity = Severity.Warning + && WarningCodes.isSuppressible code && ( match range with | None -> false diff --git a/tests/Integration/Compiler/CompilerHelpersTests.fs b/tests/Integration/Compiler/CompilerHelpersTests.fs index 5b4bd5dfa7..2e45d5b360 100644 --- a/tests/Integration/Compiler/CompilerHelpersTests.fs +++ b/tests/Integration/Compiler/CompilerHelpersTests.fs @@ -5,8 +5,21 @@ open Util.Testing open Fable.Tests.Compiler.Util open Fable.Tests.Compiler.Util.Compiler +open Fable.Transforms + let tests = testList "Compiler Helpers" [ + testCase "isSuppressible splits the code bands at FABLE0100" <| fun _ -> + // Directive warnings sit below the boundary and can never be silenced by a directive. + WarningCodes.isSuppressible (Some WarningCodes.UnknownSuppressionCode) |> equal false + WarningCodes.isSuppressible (Some WarningCodes.UnusedSuppressionDirective) |> equal false + WarningCodes.isSuppressible (Some WarningCodes.SuppressionBlockWithoutCode) |> equal false + WarningCodes.isSuppressible (Some "FABLE0099") |> equal false + WarningCodes.isSuppressible (Some "FABLE0100") |> equal true + WarningCodes.isSuppressible (Some "FABLE9999") |> equal true + // A warning with no code at all is still caught by a bare `// fable-disable-line`. + WarningCodes.isSuppressible None |> equal true + testCase "expectedVersionMatchesActual works for same major version" <| fun _ -> Fable.CompilerExt.expectedVersionMatchesActual "5.0.0" "5.0.0" |> equal true Fable.CompilerExt.expectedVersionMatchesActual "5.0.0" "5.0.1" |> equal true diff --git a/tests/Integration/Compiler/CompilerMessagesTests.fs b/tests/Integration/Compiler/CompilerMessagesTests.fs index 5f78cfa4d4..8cc4e24ed2 100644 --- a/tests/Integration/Compiler/CompilerMessagesTests.fs +++ b/tests/Integration/Compiler/CompilerMessagesTests.fs @@ -143,8 +143,8 @@ open System.Globalization let source = """ open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 -"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 +"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 """ compile source |> Assert.Are.warnings 0 @@ -154,7 +154,7 @@ open System.Globalization let source = """ open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 """ compile source |> Assert.Are.warnings 0 @@ -164,7 +164,7 @@ open System.Globalization let source = """ open System.Globalization -// fable-disable-next-line FABLE0001 +// fable-disable-next-line FABLE0100 "abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore """ compile source @@ -175,10 +175,10 @@ open System.Globalization let source = """ open System.Globalization -// fable-disable FABLE0001 +// fable-disable FABLE0100 "abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore "abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore -// fable-enable FABLE0001 +// fable-enable FABLE0100 "abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore """ compile source @@ -209,7 +209,7 @@ open System.Globalization let source = """ open System.Globalization -let s = "// fable-disable-line FABLE0001" +let s = "// fable-disable-line FABLE0100" "abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore """ compile source @@ -221,11 +221,11 @@ let s = "// fable-disable-line FABLE0001" """ open System.Globalization #if FABLE_COMPILER -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 #endif """ compile source - |> Assert.Code.noWarning "FABLE0001" + |> Assert.Code.noWarning "FABLE0100" |> ignore testCase "A warning inside a #if FABLE_COMPILER block still fires without a directive" <| fun _ -> @@ -238,7 +238,7 @@ open System.Globalization #endif """ compile source - |> Assert.Code.warning "FABLE0001" + |> Assert.Code.warning "FABLE0100" |> ignore testCase "The formatted output carries the warning code" <| fun _ -> @@ -250,12 +250,12 @@ open System.Globalization """ let formatted = compile source - |> List.filter (fun log -> log.Code = Some "FABLE0001") + |> List.filter (fun log -> log.Code = Some "FABLE0100") |> List.map (Fable.Cli.Main.Util.formatLog Compiler.Cached.projDir) match formatted with - | [] -> failwith "Expected a FABLE0001 warning" - | messages -> equal true (messages |> List.forall (fun m -> m.Contains "warning FABLE FABLE0001:")) + | [] -> failwith "Expected a FABLE0100 warning" + | messages -> equal true (messages |> List.forall (fun m -> m.Contains "warning FABLE FABLE0100:")) testCase "Errors are never suppressed, not even by a blanket fable-disable" <| fun _ -> let source = @@ -277,10 +277,10 @@ let res = jsOptions (fun o -> o.fn <- (fun i -> i)) let source = """ open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore (* fable-disable-line FABLE0001 *) +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore (* fable-disable-line FABLE0100 *) """ compile source - |> Assert.Code.noWarning "FABLE0001" + |> Assert.Code.noWarning "FABLE0100" |> ignore testCase "A trailing directive suppresses a warning spanning several lines" <| fun _ -> @@ -288,22 +288,22 @@ open System.Globalization """ open System.Globalization "abc".StartsWith( - "a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 + "a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 """ compile source - |> Assert.Code.noWarning "FABLE0001" + |> Assert.Code.noWarning "FABLE0100" |> ignore testCase "A fable-disable block with no fable-enable runs to the end of the file" <| fun _ -> let source = """ open System.Globalization -// fable-disable FABLE0001 +// fable-disable FABLE0100 "abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore "abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore """ compile source - |> Assert.Code.noWarning "FABLE0001" + |> Assert.Code.noWarning "FABLE0100" |> ignore testCase "A colon separator and a lower-case code are accepted" <| fun _ -> @@ -311,21 +311,21 @@ open System.Globalization let source = """ open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line: fable0001 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line: fable0100 """ compile source - |> Assert.Code.noWarning "FABLE0001" + |> Assert.Code.noWarning "FABLE0100" |> ignore testCase "A justification after -- is not parsed as codes" <| fun _ -> let source = """ open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 -- culture is irrelevant here +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 -- culture is irrelevant here """ compile source + |> Assert.Code.noWarning "FABLE0100" |> Assert.Code.noWarning "FABLE0001" - |> Assert.Code.noWarning "FABLE0003" |> ignore testCase "A typo'd code is reported instead of silently suppressing nothing" <| fun _ -> @@ -335,31 +335,31 @@ open System.Globalization "abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABEL0001 """ compile source - |> Assert.Code.warning "FABLE0003" |> Assert.Code.warning "FABLE0001" + |> Assert.Code.warning "FABLE0100" // The typo report is the actionable one; don't pile "and it's unused" on top of it. - |> Assert.Code.noWarning "FABLE0004" + |> Assert.Code.noWarning "FABLE0002" |> ignore testCase "A directive that suppresses nothing is reported as unused" <| fun _ -> let source = """ open System.Globalization -// fable-disable-next-line FABLE0001 +// fable-disable-next-line FABLE0100 let answer = 42 """ compile source - |> Assert.Code.warning "FABLE0004" + |> Assert.Code.warning "FABLE0002" |> ignore testCase "A directive that does its job is not reported as unused" <| fun _ -> let source = """ open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 """ compile source - |> Assert.Code.noWarning "FABLE0004" + |> Assert.Code.noWarning "FABLE0002" |> ignore testCase "A fable-disable block with no codes is reported" <| fun _ -> @@ -371,8 +371,33 @@ open System.Globalization "abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore """ compile source - |> Assert.Code.warning "FABLE0005" - |> Assert.Code.noWarning "FABLE0001" + |> Assert.Code.warning "FABLE0003" + |> Assert.Code.noWarning "FABLE0100" + |> ignore + + testCase "Directive warnings cannot be suppressed by a directive" <| fun _ -> + // FABLE0001-0099 flag comment text that is wrong to keep, so the only correct answer is to + // edit it. Silencing "this directive is broken" with another directive is self-defeating. + let source = + """ +open System.Globalization +// fable-disable-next-line FABLE0002 +// fable-disable-next-line FABLE0100 +let answer = 42 +""" + compile source + |> Assert.Code.warning "FABLE0002" + |> ignore + + testCase "A blanket fable-disable cannot suppress its own report" <| fun _ -> + let source = + """ +open System.Globalization +// fable-disable +let answer = 42 +""" + compile source + |> Assert.Code.warning "FABLE0003" |> ignore testCase "A word merely starting with a directive name is not a directive" <| fun _ -> @@ -382,19 +407,19 @@ open System.Globalization "abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disabled for now """ compile source - |> Assert.Code.warning "FABLE0001" + |> Assert.Code.warning "FABLE0100" |> ignore testCase "Directives are found in CRLF sources" <| fun _ -> let source = [ "" "open System.Globalization" - "\"abc\".StartsWith(\"a\", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0001" + "\"abc\".StartsWith(\"a\", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100" "" ] |> String.concat "\r\n" compile source - |> Assert.Code.noWarning "FABLE0001" + |> Assert.Code.noWarning "FABLE0100" |> ignore testCase "A warning from an inlined function is suppressed at its definition" <| fun _ -> @@ -404,12 +429,12 @@ open System.Globalization """ open System.Globalization let inline startsWithCulture (s: string) = - s.StartsWith("a", true, CultureInfo.InvariantCulture) // fable-disable-line FABLE0001 + s.StartsWith("a", true, CultureInfo.InvariantCulture) // fable-disable-line FABLE0100 startsWithCulture "abc" |> ignore """ compile source - |> Assert.Code.noWarning "FABLE0001" + |> Assert.Code.noWarning "FABLE0100" |> ignore testCase "A directive at the call site does not suppress an inlined function's warning" <| fun _ -> @@ -419,9 +444,9 @@ open System.Globalization let inline startsWithCulture (s: string) = s.StartsWith("a", true, CultureInfo.InvariantCulture) -startsWithCulture "abc" |> ignore // fable-disable-line FABLE0001 +startsWithCulture "abc" |> ignore // fable-disable-line FABLE0100 """ compile source - |> Assert.Code.warning "FABLE0001" + |> Assert.Code.warning "FABLE0100" |> ignore ] From a5a97b23ca5f0043fc6563856328a8d94f5fe84b Mon Sep 17 00:00:00 2001 From: Mangel Maxime Date: Thu, 13 Aug 2026 18:26:29 +0200 Subject: [PATCH 4/7] chore: save progress --- src/Fable.Transforms/Dart/Replacements.fs | 51 ++- src/Fable.Transforms/Global/WarningCodes.fs | 39 ++- src/Fable.Transforms/Python/Replacements.fs | 70 ++-- src/Fable.Transforms/Replacements.Util.fs | 9 + src/Fable.Transforms/Replacements.fs | 65 +++- src/Fable.Transforms/Rust/Replacements.fs | 57 +++- src/Fable.Transforms/Transforms.Util.fs | 3 + .../Compiler/CompilerMessagesTests.fs | 298 +++------------- .../Compiler/Fable.Tests.Compiler.fsproj | 1 + tests/Integration/Compiler/Main.fs | 1 + .../Compiler/WarningSuppressionTests.fs | 322 ++++++++++++++++++ 11 files changed, 595 insertions(+), 321 deletions(-) create mode 100644 tests/Integration/Compiler/WarningSuppressionTests.fs diff --git a/src/Fable.Transforms/Dart/Replacements.fs b/src/Fable.Transforms/Dart/Replacements.fs index 6fcf3bdbdc..0fad7b4587 100644 --- a/src/Fable.Transforms/Dart/Replacements.fs +++ b/src/Fable.Transforms/Dart/Replacements.fs @@ -1395,8 +1395,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt | "GetEnumerator", Some c, _ -> stringToCharSeq c |> getEnumerator com r t |> Some | ("Contains" | "StartsWith" | "EndsWith" as meth), Some c, arg :: _ -> if List.isMultiple args then - WarningCodes.stringSecondArgumentIgnored meth - |> addWarningWithCode com ctx.InlinePath r + WarningCodes.secondArgumentIgnored |> addWarningWithCode com ctx.InlinePath r Helper.InstanceCall(c, Naming.lowerFirst meth, t, [ arg ], ?loc = r) |> Some | ReplaceName [ "ToUpper", "toUpperCase" @@ -2086,8 +2085,8 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op let intConst = int System.Globalization.NumberStyles.Integer if style <> hexConst && style <> intConst then - $"%s{i.DeclaringEntityFullName}.%s{meth}(): NumberStyle %d{style} is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.numberStylesIgnored style + |> addWarningWithCode com ctx.InlinePath r let acceptedArgs = if meth = "Parse" then @@ -2095,10 +2094,13 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op else 3 - if List.length args > acceptedArgs then + match List.tryItem acceptedArgs args with + // InvariantCulture asks for exactly what Fable does, so there is nothing to report. + | None + | Some InvariantCulture -> () + | Some _ -> // e.g. Double.Parse(string, style, IFormatProvider) etc. - $"%s{i.DeclaringEntityFullName}.%s{meth}(): provider argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r parseCall meth str args style | ("Parse" | "TryParse") as meth, str :: _ -> @@ -2108,10 +2110,13 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op else 2 - if List.length args > acceptedArgs then + match List.tryItem acceptedArgs args with + // InvariantCulture asks for exactly what Fable does, so there is nothing to report. + | None + | Some InvariantCulture -> () + | Some _ -> // e.g. Double.Parse(string, IFormatProvider) etc. - $"%s{i.DeclaringEntityFullName}.%s{meth}(): provider argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r let style = int System.Globalization.NumberStyles.Any parseCall meth str args style @@ -2859,11 +2864,33 @@ let dates (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr optio Helper.InstanceCall(thisArg.Value, meth, t, args, ?loc = r) |> Some | meth -> + // Drops the IFormatProvider (and DateTimeStyles, where present), warning once per + // discarded argument. `Parse arg` with no extra argument discards nothing. let args = + // Passing InvariantCulture asks for exactly what Fable does, so nothing to report. + let warnProvider culture = + match culture with + | InvariantCulture -> () + | _ -> WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r + + let warnStyles styles = + match styles with + | NumberConst(NumberValue.Int32 0, _) -> () // DateTimeStyles.None: no special handling + | _ -> WarningCodes.dateTimeStylesIgnored |> addWarningWithCode com ctx.InlinePath r + match meth, args with - // Ignore IFormatProvider + | "Parse", arg :: culture :: styles :: _ -> + warnProvider culture + warnStyles styles + [ arg ] + | "Parse", arg :: culture :: _ -> + warnProvider culture + [ arg ] | "Parse", arg :: _ -> [ arg ] - | "TryParse", input :: _culture :: _styles :: defVal :: _ -> [ input; defVal ] + | "TryParse", input :: culture :: styles :: defVal :: _ -> + warnProvider culture + warnStyles styles + [ input; defVal ] | _ -> args let meth = Naming.removeGetSetPrefix meth |> Naming.lowerFirst diff --git a/src/Fable.Transforms/Global/WarningCodes.fs b/src/Fable.Transforms/Global/WarningCodes.fs index f21834e5ce..004a5809eb 100644 --- a/src/Fable.Transforms/Global/WarningCodes.fs +++ b/src/Fable.Transforms/Global/WarningCodes.fs @@ -30,7 +30,16 @@ let SuppressionBlockWithoutCode = "FABLE0003" let private CultureInfoIgnored = "FABLE0100" [] -let private StringSecondArgumentIgnored = "FABLE0101" +let private SecondArgumentIgnored = "FABLE0101" + +[] +let private FormatProviderIgnored = "FABLE0102" + +[] +let private NumberStylesIgnored = "FABLE0103" + +[] +let private DateTimeStylesIgnored = "FABLE0104" /// Every code the compiler can emit, both bands. A directive naming anything else is reported as /// a typo, so a new code MUST be added here as well as given its own function below. @@ -41,7 +50,10 @@ let knownCodes = UnusedSuppressionDirective SuppressionBlockWithoutCode CultureInfoIgnored - StringSecondArgumentIgnored + SecondArgumentIgnored + FormatProviderIgnored + NumberStylesIgnored + DateTimeStylesIgnored ] /// Can a `// fable-disable` comment silence a warning carrying this code? Codes below @@ -72,7 +84,22 @@ let suppressionBlockWithoutCode = let cultureInfoIgnored = CultureInfoIgnored, "CultureInfo argument is ignored" /// `String.Contains`/`StartsWith`/`EndsWith` called with an extra argument on the Dart target -/// (a `StringComparison`, a `CultureInfo`, ...): only the comparison itself is honored. -/// `methodName` fills in which one so the message stays specific. -let stringSecondArgumentIgnored (methodName: string) = - StringSecondArgumentIgnored, $"String.%s{methodName}: second argument is ignored" +/// (a `StringComparison`, a `CultureInfo`, ...): only the comparison itself is honored. Which +/// method it was is not spelled out - the range already points at the call. +let secondArgumentIgnored = + SecondArgumentIgnored, "Second argument is ignored: the comparison always uses the target's default rules" + +/// An `IFormatProvider`/`CultureInfo` passed to a `Parse`/`TryParse` overload. The value is +/// always parsed with the invariant rules, so a culture that changes the decimal separator or +/// the day/month order silently changes which value you get. +let formatProviderIgnored = + FormatProviderIgnored, "Format provider argument is ignored, parsing always uses the invariant culture" + +/// A `NumberStyles` value that isn't `Integer` or `HexNumber` passed to a numeric `Parse`. The +/// value is interpolated because the range can't tell you which style was discarded. +let numberStylesIgnored (style: int) = + NumberStylesIgnored, $"NumberStyles argument %d{style} is ignored" + +/// A `DateTimeStyles` value passed to a date/time `Parse`. +let dateTimeStylesIgnored = + DateTimeStylesIgnored, "DateTimeStyles argument is ignored" diff --git a/src/Fable.Transforms/Python/Replacements.fs b/src/Fable.Transforms/Python/Replacements.fs index 225b612a1d..eabe319b65 100644 --- a/src/Fable.Transforms/Python/Replacements.fs +++ b/src/Fable.Transforms/Python/Replacements.fs @@ -2322,8 +2322,8 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op let intConst = int System.Globalization.NumberStyles.Integer if style <> hexConst && style <> intConst then - $"%s{i.DeclaringEntityFullName}.%s{meth}(): NumberStyle %d{style} is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.numberStylesIgnored style + |> addWarningWithCode com ctx.InlinePath r let acceptedArgs = if meth = "Parse" then @@ -2331,10 +2331,13 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op else 3 - if List.length args > acceptedArgs then + match List.tryItem acceptedArgs args with + // InvariantCulture asks for exactly what Fable does, so there is nothing to report. + | None + | Some InvariantCulture -> () + | Some _ -> // e.g. Double.Parse(string, style, IFormatProvider) etc. - $"%s{i.DeclaringEntityFullName}.%s{meth}(): provider argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r parseCall meth str args style | ("Parse" | "TryParse") as meth, str :: _ -> @@ -2344,10 +2347,13 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op else 2 - if List.length args > acceptedArgs then + match List.tryItem acceptedArgs args with + // InvariantCulture asks for exactly what Fable does, so there is nothing to report. + | None + | Some InvariantCulture -> () + | Some _ -> // e.g. Double.Parse(string, IFormatProvider) etc. - $"%s{i.DeclaringEntityFullName}.%s{meth}(): provider argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r let style = int System.Globalization.NumberStyles.Any parseCall meth str args style @@ -2991,24 +2997,35 @@ let debug (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr optio IfThenElse(arg, makeDebugger r, unit, r) |> Some | _ -> None -let private ignoreFormatProvider com (ctx: Context) r (moduleName: string) meth args = - match meth, args with - // Ignore IFormatProvider - | "Parse", arg :: _culture :: _styles :: _ -> - addWarning com ctx.InlinePath r $"%s{moduleName}.Parse will ignore culture and styles" +/// Drops the `IFormatProvider` (and `DateTimeStyles`, where present) a date/time `Parse` was +/// given, warning once per discarded argument. Two codes rather than one bundled message so a +/// user who accepts invariant parsing can still be told about a discarded style, and vice versa. +let private ignoreFormatProvider com (ctx: Context) r meth args = + // Passing InvariantCulture asks for exactly what Fable does, so there is nothing to report. + let warnProvider culture = + match culture with + | InvariantCulture -> () + | _ -> WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r + + let warnStyles styles = + match styles with + | NumberConst(NumberValue.Int32 0, _) -> () // DateTimeStyles.None: no special handling + | _ -> WarningCodes.dateTimeStylesIgnored |> addWarningWithCode com ctx.InlinePath r + match meth, args with + | "Parse", arg :: culture :: styles :: _ -> + warnProvider culture + warnStyles styles [ arg ] - | "Parse", arg :: _culture :: _ -> - addWarning com ctx.InlinePath r $"%s{moduleName}.Parse will ignore culture" - + | "Parse", arg :: culture :: _ -> + warnProvider culture [ arg ] - | "TryParse", input :: _culture :: _styles :: defVal :: _ -> - addWarning com ctx.InlinePath r $"%s{moduleName}.TryParse will ignore culture and styles" - + | "TryParse", input :: culture :: styles :: defVal :: _ -> + warnProvider culture + warnStyles styles [ input; defVal ] - | "TryParse", input :: _culture :: defVal :: _ -> - addWarning com ctx.InlinePath r $"%s{moduleName}.TryParse will ignore culture" - + | "TryParse", input :: culture :: defVal :: _ -> + warnProvider culture [ input; defVal ] | _ -> args @@ -3059,7 +3076,7 @@ let dateOnly (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op Helper.LibCall(com, "DateOnly", meth, t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r) |> Some | meth -> - let args = ignoreFormatProvider com ctx r i.DeclaringEntityFullName meth args + let args = ignoreFormatProvider com ctx r meth args let meth = Naming.removeGetSetPrefix meth |> Naming.lowerFirst Helper.LibCall(com, "DateOnly", meth, t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r) @@ -3116,7 +3133,7 @@ let timeOnly (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op |> Some | _ -> None | meth -> - let args = ignoreFormatProvider com ctx r i.DeclaringEntityFullName meth args + let args = ignoreFormatProvider com ctx r meth args let meth = Naming.removeGetSetPrefix i.CompiledName |> Naming.lowerFirst Helper.LibCall(com, "TimeOnly", meth, t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r) @@ -3258,8 +3275,7 @@ let dates (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr optio Helper.LibCall(com, "DateOffset", "compareTo", t, [ thisArg; args.Head ], ?loc = r) ) | "TryParse" -> - let args = - ignoreFormatProvider com ctx r i.DeclaringEntityFullName i.CompiledName args + let args = ignoreFormatProvider com ctx r i.CompiledName args Helper.LibCall(com, moduleName, "tryParse", t, args, i.SignatureArgTypes, ?loc = r) |> Some @@ -3286,7 +3302,7 @@ let dates (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr optio |> Some | _ -> None | meth -> - let args = ignoreFormatProvider com ctx r i.DeclaringEntityFullName meth args + let args = ignoreFormatProvider com ctx r meth args let meth = Naming.removeGetSetPrefix meth |> Naming.lowerFirst Helper.LibCall(com, moduleName, meth, t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r) diff --git a/src/Fable.Transforms/Replacements.Util.fs b/src/Fable.Transforms/Replacements.Util.fs index ef005149e8..dda1c336ac 100644 --- a/src/Fable.Transforms/Replacements.Util.fs +++ b/src/Fable.Transforms/Replacements.Util.fs @@ -901,6 +901,15 @@ let (|ArrayOrListLiteral|_|) = | MaybeCasted(Value((NewArray(ArrayValues vals, t, _) | ListLiteral(vals, t)), _)) -> ValueSome(vals, t) | _ -> ValueNone +/// `CultureInfo.InvariantCulture`, which every target replaces with an empty object literal (see +/// `globalization`). Matching it keeps "culture is ignored" warnings quiet when the culture asked +/// for is the one Fable uses anyway. +[] +let (|InvariantCulture|_|) = + function + | MaybeCasted(ObjectExpr([], DeclaredType(entRef, _), None)) when entRef.FullName = Types.cultureInfo -> ValueSome() + | _ -> ValueNone + [] let (|IsEntity|_|) fullName = function diff --git a/src/Fable.Transforms/Replacements.fs b/src/Fable.Transforms/Replacements.fs index bdf003bc3f..9f78e52720 100644 --- a/src/Fable.Transforms/Replacements.fs +++ b/src/Fable.Transforms/Replacements.fs @@ -2635,8 +2635,8 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op let intConst = int System.Globalization.NumberStyles.Integer if style <> hexConst && style <> intConst then - $"%s{i.DeclaringEntityFullName}.%s{meth}(): NumberStyle %d{style} is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.numberStylesIgnored style + |> addWarningWithCode com ctx.InlinePath r let acceptedArgs = if meth = "Parse" then @@ -2644,10 +2644,13 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op else 3 - if List.length args > acceptedArgs then + match List.tryItem acceptedArgs args with + // InvariantCulture asks for exactly what Fable does, so there is nothing to report. + | None + | Some InvariantCulture -> () + | Some _ -> // e.g. Double.Parse(string, style, IFormatProvider) etc. - $"%s{i.DeclaringEntityFullName}.%s{meth}(): provider argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r parseCall meth str args style |> Some | ("Parse" | "TryParse") as meth, str :: _ -> @@ -2657,10 +2660,13 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op else 2 - if List.length args > acceptedArgs then + match List.tryItem acceptedArgs args with + // InvariantCulture asks for exactly what Fable does, so there is nothing to report. + | None + | Some InvariantCulture -> () + | Some _ -> // e.g. Double.Parse(string, IFormatProvider) etc. - $"%s{i.DeclaringEntityFullName}.%s{meth}(): provider argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r let style = int System.Globalization.NumberStyles.Any parseCall meth str args style |> Some @@ -3243,12 +3249,37 @@ let debug (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr optio IfThenElse(cond, makeDebugger r, unit, r) |> Some | _ -> None -let ignoreFormatProvider meth args = +/// Drops the `IFormatProvider` (and `DateTimeStyles`, where present) a date/time `Parse` was +/// given, warning once per discarded argument. `Parse arg` with no extra argument discards +/// nothing, so it must not warn. +let ignoreFormatProvider com (ctx: Context) r meth args = + // Passing InvariantCulture asks for exactly what Fable does, so there is nothing to report. + let warnProvider culture = + match culture with + | InvariantCulture -> () + | _ -> WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r + + let warnStyles styles = + match styles with + | NumberConst(NumberValue.Int32 0, _) -> () // DateTimeStyles.None: no special handling + | _ -> WarningCodes.dateTimeStylesIgnored |> addWarningWithCode com ctx.InlinePath r + match meth, args with - // Ignore IFormatProvider + | "Parse", arg :: culture :: styles :: _ -> + warnProvider culture + warnStyles styles + [ arg ] + | "Parse", arg :: culture :: _ -> + warnProvider culture + [ arg ] | "Parse", arg :: _ -> [ arg ] - | "TryParse", input :: _culture :: _styles :: defVal :: _ -> [ input; defVal ] - | "TryParse", input :: _culture :: defVal :: _ -> [ input; defVal ] + | "TryParse", input :: culture :: styles :: defVal :: _ -> + warnProvider culture + warnStyles styles + [ input; defVal ] + | "TryParse", input :: culture :: defVal :: _ -> + warnProvider culture + [ input; defVal ] | _ -> args let dateTime (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr option) (args: Expr list) = @@ -3298,7 +3329,7 @@ let dateTime (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op Helper.LibCall(com, "Date", "getTicks", t, [ thisArg.Value ], [ thisArg.Value.Type ], ?loc = r) |> Some | meth -> - let args = ignoreFormatProvider meth args + let args = ignoreFormatProvider com ctx r meth args let meth = Naming.removeGetSetPrefix meth |> Naming.lowerFirst Helper.LibCall(com, moduleName, meth, t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r) @@ -3364,7 +3395,7 @@ let dateTimeOffset (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: E Helper.LibCall(com, "DateOffset", "getUtcTicks", t, [ thisArg.Value ], [ thisArg.Value.Type ], ?loc = r) |> Some | meth -> - let args = ignoreFormatProvider meth args + let args = ignoreFormatProvider com ctx r meth args let meth = Naming.removeGetSetPrefix meth |> Naming.lowerFirst Helper.LibCall(com, moduleName, meth, t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r) @@ -3418,7 +3449,7 @@ let dateOnly (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op Helper.LibCall(com, "Date", meth, t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r) |> Some | meth -> - let args = ignoreFormatProvider meth args + let args = ignoreFormatProvider com ctx r meth args let meth = Naming.removeGetSetPrefix meth |> Naming.lowerFirst Helper.LibCall(com, "DateOnly", meth, t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r) @@ -3426,7 +3457,7 @@ let dateOnly (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op let timeSpans (com: ICompiler) (ctx: Context) r (t: Type) (i: CallInfo) (thisArg: Expr option) (args: Expr list) = let timeSpanLibCall meth args = - let args = ignoreFormatProvider meth args + let args = ignoreFormatProvider com ctx r meth args let meth = Naming.removeGetSetPrefix meth |> Naming.lowerFirst Helper.LibCall(com, "TimeSpan", meth, t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r) @@ -3540,7 +3571,7 @@ let timeOnly (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op |> Some | _ -> None | meth -> - let args = ignoreFormatProvider meth args + let args = ignoreFormatProvider com ctx r meth args let meth = Naming.removeGetSetPrefix i.CompiledName |> Naming.lowerFirst Helper.LibCall(com, "TimeOnly", meth, t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r) diff --git a/src/Fable.Transforms/Rust/Replacements.fs b/src/Fable.Transforms/Rust/Replacements.fs index fba43b41c7..6ae137197a 100644 --- a/src/Fable.Transforms/Rust/Replacements.fs +++ b/src/Fable.Transforms/Rust/Replacements.fs @@ -2171,8 +2171,8 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op let intConst = int System.Globalization.NumberStyles.Integer if style <> hexConst && style <> intConst then - $"%s{i.DeclaringEntityFullName}.%s{meth}(): NumberStyle %d{style} is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.numberStylesIgnored style + |> addWarningWithCode com ctx.InlinePath r let acceptedArgs = if meth = "Parse" then @@ -2180,10 +2180,13 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op else 3 - if List.length args > acceptedArgs then + match List.tryItem acceptedArgs args with + // InvariantCulture asks for exactly what Fable does, so there is nothing to report. + | None + | Some InvariantCulture -> () + | Some _ -> // e.g. Double.Parse(string, style, IFormatProvider) etc. - $"%s{i.DeclaringEntityFullName}.%s{meth}(): provider argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r parseCall meth str args style |> Some | ("Parse" | "TryParse") as meth, str :: _ -> @@ -2193,10 +2196,13 @@ let parseNum (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr op else 2 - if List.length args > acceptedArgs then + match List.tryItem acceptedArgs args with + // InvariantCulture asks for exactly what Fable does, so there is nothing to report. + | None + | Some InvariantCulture -> () + | Some _ -> // e.g. Double.Parse(string, IFormatProvider) etc. - $"%s{i.DeclaringEntityFullName}.%s{meth}(): provider argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r let style = int System.Globalization.NumberStyles.Any parseCall meth str args style |> Some @@ -2616,19 +2622,44 @@ let debug (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr optio | _ -> None | _ -> None -let ignoreFormatProvider compiledName args = +/// Drops the `IFormatProvider` (and `DateTimeStyles`, where present) a date/time `Parse` was +/// given, warning once per discarded argument. `Parse arg` with no extra argument discards +/// nothing, so it must not warn. `ToString` stays silent here, as it does on every other target. +let ignoreFormatProvider com (ctx: Context) r compiledName args = + // Passing InvariantCulture asks for exactly what Fable does, so there is nothing to report. + let warnProvider culture = + match culture with + | InvariantCulture -> () + | _ -> WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r + + let warnStyles styles = + match styles with + | NumberConst(NumberValue.Int32 0, _) -> () // DateTimeStyles.None: no special handling + | _ -> WarningCodes.dateTimeStylesIgnored |> addWarningWithCode com ctx.InlinePath r + match compiledName, args with - // Ignore IFormatProvider | "ToString", ExprTypeAs(String, arg) :: _ -> [ arg ] | "ToString", _ -> [ makeStrConst "" ] // default (no format string) + | "Parse", arg :: culture :: styles :: _ -> + warnProvider culture + warnStyles styles + [ arg ] + | "Parse", arg :: culture :: _ -> + warnProvider culture + [ arg ] | "Parse", arg :: _ -> [ arg ] - | "TryParse", input :: _culture :: _styles :: defVal :: _ -> [ input; defVal ] - | "TryParse", input :: _culture :: defVal :: _ -> [ input; defVal ] + | "TryParse", input :: culture :: styles :: defVal :: _ -> + warnProvider culture + warnStyles styles + [ input; defVal ] + | "TryParse", input :: culture :: defVal :: _ -> + warnProvider culture + [ input; defVal ] | _ -> args let makeMemberCall com ctx r t i moduleName memberName (thisArg: Expr option) (args: Expr list) = let memberName = Naming.removeGetSetPrefix memberName |> Naming.lowerFirst - let args = ignoreFormatProvider i.CompiledName args + let args = ignoreFormatProvider com ctx r i.CompiledName args match thisArg with | Some callee -> makeInstanceCall r t i callee memberName args diff --git a/src/Fable.Transforms/Transforms.Util.fs b/src/Fable.Transforms/Transforms.Util.fs index a4263ef996..9d9f1d5af4 100644 --- a/src/Fable.Transforms/Transforms.Util.fs +++ b/src/Fable.Transforms/Transforms.Util.fs @@ -228,6 +228,9 @@ module Types = [] let timeOnly = "System.TimeOnly" + [] + let cultureInfo = "System.Globalization.CultureInfo" + [] let timer = "System.Timers.Timer" diff --git a/tests/Integration/Compiler/CompilerMessagesTests.fs b/tests/Integration/Compiler/CompilerMessagesTests.fs index 8cc4e24ed2..ffbba39865 100644 --- a/tests/Integration/Compiler/CompilerMessagesTests.fs +++ b/tests/Integration/Compiler/CompilerMessagesTests.fs @@ -14,6 +14,7 @@ let tests = compile source |> Assert.Is.success |> ignore + testCase "Compile printfn" <| fun _ -> let source = "printfn \"Hello %s\" \"World\"" compile source @@ -25,6 +26,7 @@ let tests = compile source |> Assert.Is.Single.error |> ignore + testCase "Compiling incomplete pattern match results in warning" <| fun _ -> let source = "match None with | Some n -> 42 |> ignore" // without `ignore`: Warning: Result of Expression is implicitly ignored compile source @@ -36,6 +38,7 @@ let tests = compile source |> Assert.Exists.errorWith "This expression was expected to have type" |> ignore + testCase "Compiling incomplete pattern match results in specific warning" <| fun _ -> let source = "match None with | Some n -> 42" compile source @@ -126,119 +129,115 @@ type MyClass() = |> Assert.Is.success |> ignore - testCase "CultureInfo argument warning is not suppressed by default" <| fun _ -> - let source = - """ -open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore -""" - compile source - |> Assert.Exists.warningWith "CultureInfo argument is ignored" - |> ignore - - testCase "The same code covers both StartsWith and EndsWith call sites" <| fun _ -> - // StartsWith and EndsWith raise the same logical "CultureInfo argument is ignored" warning - // from two separate call sites in Replacements.fs, sharing WarningCodes.CultureInfoIgnored. - // One code must suppress both, otherwise the registry has failed at its only job. + testCase "Discarding a format provider on a date parse is reported" <| fun _ -> + // JS, Dart and Rust used to drop this argument silently while Python warned. Aligning them + // is a behaviour change: code that compiled quietly now raises FABLE0102. let source = """ +open System open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 -"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 +DateTime.Parse("2026-01-01", CultureInfo.GetCultureInfo "fr-FR") |> ignore """ compile source - |> Assert.Are.warnings 0 + |> Assert.Code.warning "FABLE0102" |> ignore - testCase "fable-disable-line suppresses a warning on the same line" <| fun _ -> + testCase "Passing InvariantCulture to a parse is not reported" <| fun _ -> + // Fable parses with a fixed culture-independent implementation, so InvariantCulture asks + // for exactly what it gets. Warning here would fire on every correct call - and does, if + // the exemption is removed: 96 sites across the repo's own test suites. let source = """ +open System open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 +DateTime.Parse("2026-01-01", CultureInfo.InvariantCulture) |> ignore """ compile source - |> Assert.Are.warnings 0 + |> Assert.Code.noWarning "FABLE0102" |> ignore - testCase "fable-disable-next-line suppresses a warning on the following line" <| fun _ -> + testCase "A discarded DateTimeStyles is reported even when the culture is exempt" <| fun _ -> + // Two codes rather than one bundled message: the style is still discarded regardless of + // which culture was passed, so exempting the culture must not silence it. let source = """ +open System open System.Globalization -// fable-disable-next-line FABLE0100 -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +DateTime.Parse("2026-01-01", CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal) |> ignore """ compile source - |> Assert.Are.warnings 0 + |> Assert.Code.warning "FABLE0104" + |> Assert.Code.noWarning "FABLE0102" |> ignore - testCase "fable-disable/fable-enable suppresses warnings in a block" <| fun _ -> + testCase "DateTimeStyles.None is not reported" <| fun _ -> + // `None` means "no special handling", which is what Fable does - same exemption as + // InvariantCulture. Every one of the 12 sites in tests/Js passes exactly this. let source = """ +open System open System.Globalization -// fable-disable FABLE0100 -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore -"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore -// fable-enable FABLE0100 -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +DateTime.Parse("2026-01-01", CultureInfo.InvariantCulture, DateTimeStyles.None) |> ignore """ compile source - |> Assert.Are.warnings 1 + |> Assert.Code.noWarning "FABLE0102" + |> Assert.Code.noWarning "FABLE0104" |> ignore - testCase "A mismatched code does not suppress the warning" <| fun _ -> + testCase "A numeric parse given InvariantCulture is not reported" <| fun _ -> let source = """ +open System open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line SOME_OTHER_CODE +Double.Parse("10.5", CultureInfo.InvariantCulture) |> ignore """ compile source - |> Assert.Exists.warningWith "CultureInfo argument is ignored" + |> Assert.Code.noWarning "FABLE0102" |> ignore - testCase "A bare fable-disable-line suppresses regardless of code" <| fun _ -> + testCase "A numeric parse given a real culture is reported" <| fun _ -> let source = """ +open System open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line +Double.Parse("10.5", CultureInfo.GetCultureInfo "fr-FR") |> ignore """ compile source - |> Assert.Are.warnings 0 + |> Assert.Code.warning "FABLE0102" |> ignore - testCase "A string literal that looks like a directive is not treated as one" <| fun _ -> + testCase "A real culture and a style are both reported" <| fun _ -> let source = """ +open System open System.Globalization -let s = "// fable-disable-line FABLE0100" -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +DateTime.Parse("2026-01-01", CultureInfo.GetCultureInfo "fr-FR", DateTimeStyles.AssumeUniversal) |> ignore """ compile source - |> Assert.Exists.warningWith "CultureInfo argument is ignored" + |> Assert.Code.warning "FABLE0102" + |> Assert.Code.warning "FABLE0104" |> ignore - testCase "A directive inside a #if FABLE_COMPILER block is honoured" <| fun _ -> + testCase "A date parse with no extra argument discards nothing and is silent" <| fun _ -> let source = """ -open System.Globalization -#if FABLE_COMPILER -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 -#endif +open System +DateTime.Parse("2026-01-01") |> ignore """ compile source - |> Assert.Code.noWarning "FABLE0100" + |> Assert.Code.noWarning "FABLE0102" + |> Assert.Code.noWarning "FABLE0104" |> ignore - testCase "A warning inside a #if FABLE_COMPILER block still fires without a directive" <| fun _ -> - // Guards the test above from passing vacuously because the block was compiled out. + testCase "A discarded NumberStyles is reported" <| fun _ -> let source = """ +open System open System.Globalization -#if FABLE_COMPILER -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore -#endif +Double.Parse("1.5", NumberStyles.Currency, CultureInfo.InvariantCulture) |> ignore """ compile source - |> Assert.Code.warning "FABLE0100" + |> Assert.Code.warning "FABLE0103" |> ignore testCase "The formatted output carries the warning code" <| fun _ -> @@ -256,197 +255,4 @@ open System.Globalization match formatted with | [] -> failwith "Expected a FABLE0100 warning" | messages -> equal true (messages |> List.forall (fun m -> m.Contains "warning FABLE FABLE0100:")) - - testCase "Errors are never suppressed, not even by a blanket fable-disable" <| fun _ -> - let source = - """ -open Fable.Core.JsInterop - -type Response = - abstract fn: int -> int - abstract prop: bool with get, set - -// fable-disable -let res = jsOptions (fun o -> o.fn <- (fun i -> i)) -""" - compile source - |> Assert.Exists.errorWith "Cannot set a non-property member in 'jsOptions'" - |> ignore - - testCase "A directive written as a block comment is honoured" <| fun _ -> - let source = - """ -open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore (* fable-disable-line FABLE0100 *) -""" - compile source - |> Assert.Code.noWarning "FABLE0100" - |> ignore - - testCase "A trailing directive suppresses a warning spanning several lines" <| fun _ -> - let source = - """ -open System.Globalization -"abc".StartsWith( - "a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 -""" - compile source - |> Assert.Code.noWarning "FABLE0100" - |> ignore - - testCase "A fable-disable block with no fable-enable runs to the end of the file" <| fun _ -> - let source = - """ -open System.Globalization -// fable-disable FABLE0100 -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore -"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore -""" - compile source - |> Assert.Code.noWarning "FABLE0100" - |> ignore - - testCase "A colon separator and a lower-case code are accepted" <| fun _ -> - // What people coming from `# noqa: E501` and `@ts-ignore` will write. - let source = - """ -open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line: fable0100 -""" - compile source - |> Assert.Code.noWarning "FABLE0100" - |> ignore - - testCase "A justification after -- is not parsed as codes" <| fun _ -> - let source = - """ -open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 -- culture is irrelevant here -""" - compile source - |> Assert.Code.noWarning "FABLE0100" - |> Assert.Code.noWarning "FABLE0001" - |> ignore - - testCase "A typo'd code is reported instead of silently suppressing nothing" <| fun _ -> - let source = - """ -open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABEL0001 -""" - compile source - |> Assert.Code.warning "FABLE0001" - |> Assert.Code.warning "FABLE0100" - // The typo report is the actionable one; don't pile "and it's unused" on top of it. - |> Assert.Code.noWarning "FABLE0002" - |> ignore - - testCase "A directive that suppresses nothing is reported as unused" <| fun _ -> - let source = - """ -open System.Globalization -// fable-disable-next-line FABLE0100 -let answer = 42 -""" - compile source - |> Assert.Code.warning "FABLE0002" - |> ignore - - testCase "A directive that does its job is not reported as unused" <| fun _ -> - let source = - """ -open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 -""" - compile source - |> Assert.Code.noWarning "FABLE0002" - |> ignore - - testCase "A fable-disable block with no codes is reported" <| fun _ -> - // It would otherwise silence every Fable warning to the end of the file, unnoticed. - let source = - """ -open System.Globalization -// fable-disable -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore -""" - compile source - |> Assert.Code.warning "FABLE0003" - |> Assert.Code.noWarning "FABLE0100" - |> ignore - - testCase "Directive warnings cannot be suppressed by a directive" <| fun _ -> - // FABLE0001-0099 flag comment text that is wrong to keep, so the only correct answer is to - // edit it. Silencing "this directive is broken" with another directive is self-defeating. - let source = - """ -open System.Globalization -// fable-disable-next-line FABLE0002 -// fable-disable-next-line FABLE0100 -let answer = 42 -""" - compile source - |> Assert.Code.warning "FABLE0002" - |> ignore - - testCase "A blanket fable-disable cannot suppress its own report" <| fun _ -> - let source = - """ -open System.Globalization -// fable-disable -let answer = 42 -""" - compile source - |> Assert.Code.warning "FABLE0003" - |> ignore - - testCase "A word merely starting with a directive name is not a directive" <| fun _ -> - let source = - """ -open System.Globalization -"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disabled for now -""" - compile source - |> Assert.Code.warning "FABLE0100" - |> ignore - - testCase "Directives are found in CRLF sources" <| fun _ -> - let source = - [ "" - "open System.Globalization" - "\"abc\".StartsWith(\"a\", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100" - "" ] - |> String.concat "\r\n" - - compile source - |> Assert.Code.noWarning "FABLE0100" - |> ignore - - testCase "A warning from an inlined function is suppressed at its definition" <| fun _ -> - // The warning is attributed to the file the inline function is *defined* in, so that is - // where the directive has to go - the call site can't suppress it. - let source = - """ -open System.Globalization -let inline startsWithCulture (s: string) = - s.StartsWith("a", true, CultureInfo.InvariantCulture) // fable-disable-line FABLE0100 - -startsWithCulture "abc" |> ignore -""" - compile source - |> Assert.Code.noWarning "FABLE0100" - |> ignore - - testCase "A directive at the call site does not suppress an inlined function's warning" <| fun _ -> - let source = - """ -open System.Globalization -let inline startsWithCulture (s: string) = - s.StartsWith("a", true, CultureInfo.InvariantCulture) - -startsWithCulture "abc" |> ignore // fable-disable-line FABLE0100 -""" - compile source - |> Assert.Code.warning "FABLE0100" - |> ignore ] diff --git a/tests/Integration/Compiler/Fable.Tests.Compiler.fsproj b/tests/Integration/Compiler/Fable.Tests.Compiler.fsproj index 49253cae8a..03e4dff270 100644 --- a/tests/Integration/Compiler/Fable.Tests.Compiler.fsproj +++ b/tests/Integration/Compiler/Fable.Tests.Compiler.fsproj @@ -23,6 +23,7 @@ + diff --git a/tests/Integration/Compiler/Main.fs b/tests/Integration/Compiler/Main.fs index 18c778cfa4..e1d3bfbf7d 100644 --- a/tests/Integration/Compiler/Main.fs +++ b/tests/Integration/Compiler/Main.fs @@ -6,6 +6,7 @@ open Expecto let allTests = [ CompilerMessages.tests + WarningSuppression.tests AnonRecordInInterface.tests CompilerHelpers.tests Inflate.tests diff --git a/tests/Integration/Compiler/WarningSuppressionTests.fs b/tests/Integration/Compiler/WarningSuppressionTests.fs new file mode 100644 index 0000000000..bda6ca3d3b --- /dev/null +++ b/tests/Integration/Compiler/WarningSuppressionTests.fs @@ -0,0 +1,322 @@ +module Fable.Tests.Compiler.WarningSuppression + +open Fable.Core +open Util.Testing +open Fable.Tests.Compiler.Util +open Fable.Tests.Compiler.Util.Compiler + +let private compile source = Compiler.Cached.compile Compiler.Settings.standard source + +/// Exercises the `// fable-disable` machinery itself - directive parsing, block scoping, which +/// warnings it can reach, and the FABLE0001-band reports about the directives. FABLE0100 stands +/// in as the subject warning throughout; what is under test is the mechanism, not that code. +let tests = + testList "Warning Suppression" [ + testCase "CultureInfo argument warning is not suppressed by default" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Exists.warningWith "CultureInfo argument is ignored" + |> ignore + + testCase "The same code covers both StartsWith and EndsWith call sites" <| fun _ -> + // StartsWith and EndsWith raise the same logical "CultureInfo argument is ignored" warning + // from two separate call sites in Replacements.fs, sharing WarningCodes.CultureInfoIgnored. + // One code must suppress both, otherwise the registry has failed at its only job. + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 +"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 +""" + compile source + |> Assert.Are.warnings 0 + |> ignore + + testCase "fable-disable-line suppresses a warning on the same line" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 +""" + compile source + |> Assert.Are.warnings 0 + |> ignore + + testCase "fable-disable-next-line suppresses a warning on the following line" <| fun _ -> + let source = + """ +open System.Globalization +// fable-disable-next-line FABLE0100 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Are.warnings 0 + |> ignore + + testCase "fable-disable/fable-enable suppresses warnings in a block" <| fun _ -> + let source = + """ +open System.Globalization +// fable-disable FABLE0100 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore +// fable-enable FABLE0100 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Are.warnings 1 + |> ignore + + testCase "A mismatched code does not suppress the warning" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line SOME_OTHER_CODE +""" + compile source + |> Assert.Exists.warningWith "CultureInfo argument is ignored" + |> ignore + + testCase "A bare fable-disable-line suppresses regardless of code" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line +""" + compile source + |> Assert.Are.warnings 0 + |> ignore + + testCase "A string literal that looks like a directive is not treated as one" <| fun _ -> + let source = + """ +open System.Globalization +let s = "// fable-disable-line FABLE0100" +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Exists.warningWith "CultureInfo argument is ignored" + |> ignore + + testCase "A directive inside a #if FABLE_COMPILER block is honoured" <| fun _ -> + let source = + """ +open System.Globalization +#if FABLE_COMPILER +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 +#endif +""" + compile source + |> Assert.Code.noWarning "FABLE0100" + |> ignore + + testCase "A warning inside a #if FABLE_COMPILER block still fires without a directive" <| fun _ -> + // Guards the test above from passing vacuously because the block was compiled out. + let source = + """ +open System.Globalization +#if FABLE_COMPILER +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +#endif +""" + compile source + |> Assert.Code.warning "FABLE0100" + |> ignore + + testCase "Errors are never suppressed, not even by a blanket fable-disable" <| fun _ -> + let source = + """ +open Fable.Core.JsInterop + +type Response = + abstract fn: int -> int + abstract prop: bool with get, set + +// fable-disable +let res = jsOptions (fun o -> o.fn <- (fun i -> i)) +""" + compile source + |> Assert.Exists.errorWith "Cannot set a non-property member in 'jsOptions'" + |> ignore + + testCase "A directive written as a block comment is honoured" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore (* fable-disable-line FABLE0100 *) +""" + compile source + |> Assert.Code.noWarning "FABLE0100" + |> ignore + + testCase "A trailing directive suppresses a warning spanning several lines" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith( + "a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 +""" + compile source + |> Assert.Code.noWarning "FABLE0100" + |> ignore + + testCase "A fable-disable block with no fable-enable runs to the end of the file" <| fun _ -> + let source = + """ +open System.Globalization +// fable-disable FABLE0100 +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +"abc".EndsWith("c", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Code.noWarning "FABLE0100" + |> ignore + + testCase "A colon separator and a lower-case code are accepted" <| fun _ -> + // What people coming from `# noqa: E501` and `@ts-ignore` will write. + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line: fable0100 +""" + compile source + |> Assert.Code.noWarning "FABLE0100" + |> ignore + + testCase "A justification after -- is not parsed as codes" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 -- culture is irrelevant here +""" + compile source + |> Assert.Code.noWarning "FABLE0100" + |> Assert.Code.noWarning "FABLE0001" + |> ignore + + testCase "A typo'd code is reported instead of silently suppressing nothing" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABEL0001 +""" + compile source + |> Assert.Code.warning "FABLE0001" + |> Assert.Code.warning "FABLE0100" + // The typo report is the actionable one; don't pile "and it's unused" on top of it. + |> Assert.Code.noWarning "FABLE0002" + |> ignore + + testCase "A directive that suppresses nothing is reported as unused" <| fun _ -> + let source = + """ +open System.Globalization +// fable-disable-next-line FABLE0100 +let answer = 42 +""" + compile source + |> Assert.Code.warning "FABLE0002" + |> ignore + + testCase "A directive that does its job is not reported as unused" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100 +""" + compile source + |> Assert.Code.noWarning "FABLE0002" + |> ignore + + testCase "A fable-disable block with no codes is reported" <| fun _ -> + // It would otherwise silence every Fable warning to the end of the file, unnoticed. + let source = + """ +open System.Globalization +// fable-disable +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0003" + |> Assert.Code.noWarning "FABLE0100" + |> ignore + + testCase "Directive warnings cannot be suppressed by a directive" <| fun _ -> + // FABLE0001-0099 flag comment text that is wrong to keep, so the only correct answer is to + // edit it. Silencing "this directive is broken" with another directive is self-defeating. + let source = + """ +open System.Globalization +// fable-disable-next-line FABLE0002 +// fable-disable-next-line FABLE0100 +let answer = 42 +""" + compile source + |> Assert.Code.warning "FABLE0002" + |> ignore + + testCase "A blanket fable-disable cannot suppress its own report" <| fun _ -> + let source = + """ +open System.Globalization +// fable-disable +let answer = 42 +""" + compile source + |> Assert.Code.warning "FABLE0003" + |> ignore + + testCase "A word merely starting with a directive name is not a directive" <| fun _ -> + let source = + """ +open System.Globalization +"abc".StartsWith("a", true, CultureInfo.InvariantCulture) |> ignore // fable-disabled for now +""" + compile source + |> Assert.Code.warning "FABLE0100" + |> ignore + + testCase "Directives are found in CRLF sources" <| fun _ -> + let source = + [ "" + "open System.Globalization" + "\"abc\".StartsWith(\"a\", true, CultureInfo.InvariantCulture) |> ignore // fable-disable-line FABLE0100" + "" ] + |> String.concat "\r\n" + + compile source + |> Assert.Code.noWarning "FABLE0100" + |> ignore + + testCase "A warning from an inlined function is suppressed at its definition" <| fun _ -> + // The warning is attributed to the file the inline function is *defined* in, so that is + // where the directive has to go - the call site can't suppress it. + let source = + """ +open System.Globalization +let inline startsWithCulture (s: string) = + s.StartsWith("a", true, CultureInfo.InvariantCulture) // fable-disable-line FABLE0100 + +startsWithCulture "abc" |> ignore +""" + compile source + |> Assert.Code.noWarning "FABLE0100" + |> ignore + + testCase "A directive at the call site does not suppress an inlined function's warning" <| fun _ -> + let source = + """ +open System.Globalization +let inline startsWithCulture (s: string) = + s.StartsWith("a", true, CultureInfo.InvariantCulture) + +startsWithCulture "abc" |> ignore // fable-disable-line FABLE0100 +""" + compile source + |> Assert.Code.warning "FABLE0100" + |> ignore + ] From 9418933a0363bc9f3587b40f667e8e06d71d6d78 Mon Sep 17 00:00:00 2001 From: Mangel Maxime Date: Thu, 13 Aug 2026 20:58:46 +0200 Subject: [PATCH 5/7] chore: extract warnings tests --- .../Compiler/CompilerMessagesTests.fs | 111 --------------- .../Compiler/Fable.Tests.Compiler.fsproj | 1 + .../Compiler/IgnoredArgumentTests.fs | 129 ++++++++++++++++++ tests/Integration/Compiler/Main.fs | 1 + 4 files changed, 131 insertions(+), 111 deletions(-) create mode 100644 tests/Integration/Compiler/IgnoredArgumentTests.fs diff --git a/tests/Integration/Compiler/CompilerMessagesTests.fs b/tests/Integration/Compiler/CompilerMessagesTests.fs index ffbba39865..127cf4b8c5 100644 --- a/tests/Integration/Compiler/CompilerMessagesTests.fs +++ b/tests/Integration/Compiler/CompilerMessagesTests.fs @@ -129,117 +129,6 @@ type MyClass() = |> Assert.Is.success |> ignore - testCase "Discarding a format provider on a date parse is reported" <| fun _ -> - // JS, Dart and Rust used to drop this argument silently while Python warned. Aligning them - // is a behaviour change: code that compiled quietly now raises FABLE0102. - let source = - """ -open System -open System.Globalization -DateTime.Parse("2026-01-01", CultureInfo.GetCultureInfo "fr-FR") |> ignore -""" - compile source - |> Assert.Code.warning "FABLE0102" - |> ignore - - testCase "Passing InvariantCulture to a parse is not reported" <| fun _ -> - // Fable parses with a fixed culture-independent implementation, so InvariantCulture asks - // for exactly what it gets. Warning here would fire on every correct call - and does, if - // the exemption is removed: 96 sites across the repo's own test suites. - let source = - """ -open System -open System.Globalization -DateTime.Parse("2026-01-01", CultureInfo.InvariantCulture) |> ignore -""" - compile source - |> Assert.Code.noWarning "FABLE0102" - |> ignore - - testCase "A discarded DateTimeStyles is reported even when the culture is exempt" <| fun _ -> - // Two codes rather than one bundled message: the style is still discarded regardless of - // which culture was passed, so exempting the culture must not silence it. - let source = - """ -open System -open System.Globalization -DateTime.Parse("2026-01-01", CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal) |> ignore -""" - compile source - |> Assert.Code.warning "FABLE0104" - |> Assert.Code.noWarning "FABLE0102" - |> ignore - - testCase "DateTimeStyles.None is not reported" <| fun _ -> - // `None` means "no special handling", which is what Fable does - same exemption as - // InvariantCulture. Every one of the 12 sites in tests/Js passes exactly this. - let source = - """ -open System -open System.Globalization -DateTime.Parse("2026-01-01", CultureInfo.InvariantCulture, DateTimeStyles.None) |> ignore -""" - compile source - |> Assert.Code.noWarning "FABLE0102" - |> Assert.Code.noWarning "FABLE0104" - |> ignore - - testCase "A numeric parse given InvariantCulture is not reported" <| fun _ -> - let source = - """ -open System -open System.Globalization -Double.Parse("10.5", CultureInfo.InvariantCulture) |> ignore -""" - compile source - |> Assert.Code.noWarning "FABLE0102" - |> ignore - - testCase "A numeric parse given a real culture is reported" <| fun _ -> - let source = - """ -open System -open System.Globalization -Double.Parse("10.5", CultureInfo.GetCultureInfo "fr-FR") |> ignore -""" - compile source - |> Assert.Code.warning "FABLE0102" - |> ignore - - testCase "A real culture and a style are both reported" <| fun _ -> - let source = - """ -open System -open System.Globalization -DateTime.Parse("2026-01-01", CultureInfo.GetCultureInfo "fr-FR", DateTimeStyles.AssumeUniversal) |> ignore -""" - compile source - |> Assert.Code.warning "FABLE0102" - |> Assert.Code.warning "FABLE0104" - |> ignore - - testCase "A date parse with no extra argument discards nothing and is silent" <| fun _ -> - let source = - """ -open System -DateTime.Parse("2026-01-01") |> ignore -""" - compile source - |> Assert.Code.noWarning "FABLE0102" - |> Assert.Code.noWarning "FABLE0104" - |> ignore - - testCase "A discarded NumberStyles is reported" <| fun _ -> - let source = - """ -open System -open System.Globalization -Double.Parse("1.5", NumberStyles.Currency, CultureInfo.InvariantCulture) |> ignore -""" - compile source - |> Assert.Code.warning "FABLE0103" - |> ignore - testCase "The formatted output carries the warning code" <| fun _ -> // Without this there is no way to discover which code to put in a directive. let source = diff --git a/tests/Integration/Compiler/Fable.Tests.Compiler.fsproj b/tests/Integration/Compiler/Fable.Tests.Compiler.fsproj index 03e4dff270..6098d46d36 100644 --- a/tests/Integration/Compiler/Fable.Tests.Compiler.fsproj +++ b/tests/Integration/Compiler/Fable.Tests.Compiler.fsproj @@ -24,6 +24,7 @@ + diff --git a/tests/Integration/Compiler/IgnoredArgumentTests.fs b/tests/Integration/Compiler/IgnoredArgumentTests.fs new file mode 100644 index 0000000000..c07d51d347 --- /dev/null +++ b/tests/Integration/Compiler/IgnoredArgumentTests.fs @@ -0,0 +1,129 @@ +module Fable.Tests.Compiler.IgnoredArgument + +open Fable.Core +open Util.Testing +open Fable.Tests.Compiler.Util +open Fable.Tests.Compiler.Util.Compiler + +let private compile source = Compiler.Cached.compile Compiler.Settings.standard source + +(* + Verify that warnings are generated when needed, and skipped in the scenario where Fable + has the same behavior as .NET. + + The later makes the logs less verbose and warnings more meaningful +*) + +let tests = + testList "Ignored Arguments" [ + testCase "Discarding a format provider on a date parse is reported" <| fun _ -> + // JS, Dart and Rust used to drop this argument silently while Python warned. Aligning them + // is a behaviour change: code that compiled quietly now raises FABLE0102. + let source = + """ +open System +open System.Globalization +DateTime.Parse("2026-01-01", CultureInfo.GetCultureInfo "fr-FR") |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0102" + |> ignore + + testCase "Passing InvariantCulture to a parse is not reported" <| fun _ -> + // Fable parses with a fixed culture-independent implementation, so InvariantCulture asks + // for exactly what it gets. Warning here would fire on every correct call - and does, if + // the exemption is removed: 96 sites across the repo's own test suites. + let source = + """ +open System +open System.Globalization +DateTime.Parse("2026-01-01", CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Code.noWarning "FABLE0102" + |> ignore + + testCase "A discarded DateTimeStyles is reported even when the culture is exempt" <| fun _ -> + // Two codes rather than one bundled message: the style is still discarded regardless of + // which culture was passed, so exempting the culture must not silence it. + let source = + """ +open System +open System.Globalization +DateTime.Parse("2026-01-01", CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal) |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0104" + |> Assert.Code.noWarning "FABLE0102" + |> ignore + + testCase "DateTimeStyles.None is not reported" <| fun _ -> + // `None` means "no special handling", which is what Fable does - same exemption as + // InvariantCulture. Every one of the 12 sites in tests/Js passes exactly this. + let source = + """ +open System +open System.Globalization +DateTime.Parse("2026-01-01", CultureInfo.InvariantCulture, DateTimeStyles.None) |> ignore +""" + compile source + |> Assert.Code.noWarning Fable.tran "FABLE0102" + |> Assert.Code.noWarning "FABLE0104" + |> ignore + + testCase "A numeric parse given InvariantCulture is not reported" <| fun _ -> + let source = + """ +open System +open System.Globalization +Double.Parse("10.5", CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Code.noWarning "FABLE0102" + |> ignore + + testCase "A numeric parse given a real culture is reported" <| fun _ -> + let source = + """ +open System +open System.Globalization +Double.Parse("10.5", CultureInfo.GetCultureInfo "fr-FR") |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0102" + |> ignore + + testCase "A real culture and a style are both reported" <| fun _ -> + let source = + """ +open System +open System.Globalization +DateTime.Parse("2026-01-01", CultureInfo.GetCultureInfo "fr-FR", DateTimeStyles.AssumeUniversal) |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0102" + |> Assert.Code.warning "FABLE0104" + |> ignore + + testCase "A date parse with no extra argument discards nothing and is silent" <| fun _ -> + let source = + """ +open System +DateTime.Parse("2026-01-01") |> ignore +""" + compile source + |> Assert.Code.noWarning "FABLE0102" + |> Assert.Code.noWarning "FABLE0104" + |> ignore + + testCase "A discarded NumberStyles is reported" <| fun _ -> + let source = + """ +open System +open System.Globalization +Double.Parse("1.5", NumberStyles.Currency, CultureInfo.InvariantCulture) |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0103" + |> ignore + ] diff --git a/tests/Integration/Compiler/Main.fs b/tests/Integration/Compiler/Main.fs index e1d3bfbf7d..d1453a7627 100644 --- a/tests/Integration/Compiler/Main.fs +++ b/tests/Integration/Compiler/Main.fs @@ -7,6 +7,7 @@ let allTests = [ CompilerMessages.tests WarningSuppression.tests + IgnoredArgument.tests AnonRecordInInterface.tests CompilerHelpers.tests Inflate.tests From 146e5044dee70dbdf3e817847916b44a074ba71f Mon Sep 17 00:00:00 2001 From: Mangel Maxime Date: Thu, 13 Aug 2026 21:44:13 +0200 Subject: [PATCH 6/7] test: safe guard in place --- src/Fable.Transforms/Global/WarningCodes.fs | 95 +++++++------------ .../Compiler/CompilerHelpersTests.fs | 54 ++++++++++- .../Compiler/IgnoredArgumentTests.fs | 2 +- 3 files changed, 88 insertions(+), 63 deletions(-) diff --git a/src/Fable.Transforms/Global/WarningCodes.fs b/src/Fable.Transforms/Global/WarningCodes.fs index 004a5809eb..60fcb8b9d7 100644 --- a/src/Fable.Transforms/Global/WarningCodes.fs +++ b/src/Fable.Transforms/Global/WarningCodes.fs @@ -6,56 +6,6 @@ *) module Fable.Transforms.WarningCodes -(* - FABLE0001-0099: about Fable's own directives. Never suppressible. -*) - -/// A `fable-disable*` directive named something that isn't in this registry - most likely a typo. -[] -let UnknownSuppressionCode = "FABLE0001" - -/// A `fable-disable*` directive that never suppressed anything, so it can be deleted. -[] -let UnusedSuppressionDirective = "FABLE0002" - -/// A bare `// fable-disable` block, which would silence every Fable warning up to end of file. -[] -let SuppressionBlockWithoutCode = "FABLE0003" - -(* - FABLE0100 and up: about the compiled code. Suppressible. -*) - -[] -let private CultureInfoIgnored = "FABLE0100" - -[] -let private SecondArgumentIgnored = "FABLE0101" - -[] -let private FormatProviderIgnored = "FABLE0102" - -[] -let private NumberStylesIgnored = "FABLE0103" - -[] -let private DateTimeStylesIgnored = "FABLE0104" - -/// Every code the compiler can emit, both bands. A directive naming anything else is reported as -/// a typo, so a new code MUST be added here as well as given its own function below. -let knownCodes = - set - [ - UnknownSuppressionCode - UnusedSuppressionDirective - SuppressionBlockWithoutCode - CultureInfoIgnored - SecondArgumentIgnored - FormatProviderIgnored - NumberStylesIgnored - DateTimeStylesIgnored - ] - /// Can a `// fable-disable` comment silence a warning carrying this code? Codes below /// `FABLE0100` describe the directives themselves and never can - see the band rules above. /// This is what makes the banding an invariant rather than a naming convention: it holds @@ -68,38 +18,65 @@ let isSuppressible (code: string option) = // Every code is `FABLE` plus four digits, so ordinal comparison orders them numerically. | Some code -> System.String.CompareOrdinal(code, "FABLE0100") >= 0 +(* + FABLE0001-0099: about Fable's own directives. Never suppressible. +*) + +/// A `fable-disable*` directive named something that isn't in this registry - most likely a typo. let unknownSuppressionCode (code: string) = - UnknownSuppressionCode, $"Unknown warning code '%s{code}' in a fable-disable directive" + "FABLE0001", $"Unknown warning code '%s{code}' in a fable-disable directive" +/// A `fable-disable*` directive that never suppressed anything, so it can be deleted. let unusedSuppressionDirective = - UnusedSuppressionDirective, "This fable-disable directive doesn't suppress anything" + "FABLE0002", "This fable-disable directive doesn't suppress anything" +/// A bare `// fable-disable` block, which would silence every Fable warning up to end of file. let suppressionBlockWithoutCode = - SuppressionBlockWithoutCode, + "FABLE0003", "A 'fable-disable' block must list the warning codes it suppresses, otherwise it silences every Fable warning until the end of the file" +(* + FABLE0100 and up: about the compiled code. Suppressible. +*) + /// `String.StartsWith`/`EndsWith` with a `CultureInfo` argument: the comparison always runs /// with the target's default culture rules, the argument is accepted but has no effect. /// Used in both the JS/TS and Python replacements. -let cultureInfoIgnored = CultureInfoIgnored, "CultureInfo argument is ignored" +let cultureInfoIgnored = "FABLE0100", "CultureInfo argument is ignored" /// `String.Contains`/`StartsWith`/`EndsWith` called with an extra argument on the Dart target /// (a `StringComparison`, a `CultureInfo`, ...): only the comparison itself is honored. Which /// method it was is not spelled out - the range already points at the call. let secondArgumentIgnored = - SecondArgumentIgnored, "Second argument is ignored: the comparison always uses the target's default rules" + "FABLE0101", "Second argument is ignored: the comparison always uses the target's default rules" /// An `IFormatProvider`/`CultureInfo` passed to a `Parse`/`TryParse` overload. The value is /// always parsed with the invariant rules, so a culture that changes the decimal separator or /// the day/month order silently changes which value you get. let formatProviderIgnored = - FormatProviderIgnored, "Format provider argument is ignored, parsing always uses the invariant culture" + "FABLE0102", "Format provider argument is ignored, parsing always uses the invariant culture" /// A `NumberStyles` value that isn't `Integer` or `HexNumber` passed to a numeric `Parse`. The /// value is interpolated because the range can't tell you which style was discarded. let numberStylesIgnored (style: int) = - NumberStylesIgnored, $"NumberStyles argument %d{style} is ignored" + "FABLE0103", $"NumberStyles argument %d{style} is ignored" /// A `DateTimeStyles` value passed to a date/time `Parse`. -let dateTimeStylesIgnored = - DateTimeStylesIgnored, "DateTimeStyles argument is ignored" +let dateTimeStylesIgnored = "FABLE0104", "DateTimeStyles argument is ignored" + +/// Every code the compiler can emit, both bands. A directive naming anything else is reported as +/// a typo, so a new warning MUST be added to this list as well as defined above. +let knownCodes = + [ + // The two parameterised warnings get a throwaway argument; only the code is read. + unknownSuppressionCode "" + unusedSuppressionDirective + suppressionBlockWithoutCode + cultureInfoIgnored + secondArgumentIgnored + formatProviderIgnored + numberStylesIgnored 0 + dateTimeStylesIgnored + ] + |> List.map fst + |> Set.ofList diff --git a/tests/Integration/Compiler/CompilerHelpersTests.fs b/tests/Integration/Compiler/CompilerHelpersTests.fs index 2e45d5b360..a3a2bfa393 100644 --- a/tests/Integration/Compiler/CompilerHelpersTests.fs +++ b/tests/Integration/Compiler/CompilerHelpersTests.fs @@ -5,15 +5,63 @@ open Util.Testing open Fable.Tests.Compiler.Util open Fable.Tests.Compiler.Util.Compiler +open System +open System.Reflection open Fable.Transforms +/// Every warning code `WarningCodes` can emit, read from the module itself so that this test +/// has no list of its own to fall out of date. +let private codesTheRegistryCanProduce () = + let flags = BindingFlags.Public ||| BindingFlags.Static + let moduleType = typeof.Assembly.GetType("Fable.Transforms.WarningCodes") + let codeOf (pair: obj) = fst (pair :?> string * string) + + // The parameterised warnings interpolate their argument into the message, never into the + // code, so anything of the right type will do. + let throwaway (p: ParameterInfo) = + if p.ParameterType = typeof then box "" + elif p.ParameterType.IsValueType then Activator.CreateInstance p.ParameterType + else null + + // A warning is anything shaped `string * string`: plain ones compile to static properties, + // parameterised ones to static methods. Nothing else in the module has that shape. + let fromValues = + moduleType.GetProperties(flags) + |> Array.filter (fun p -> p.PropertyType = typeof) + |> Array.map (fun p -> codeOf (p.GetValue(null))) + + let fromFactories = + moduleType.GetMethods(flags) + |> Array.filter (fun m -> not m.IsSpecialName && m.ReturnType = typeof) + |> Array.map (fun m -> codeOf (m.Invoke(null, m.GetParameters() |> Array.map throwaway))) + + Set.ofArray (Array.append fromValues fromFactories) + let tests = testList "Compiler Helpers" [ + // The goal of this test is to act as a safe guard for us, so we remember to add + // warning code to the knownCodes list. + testCase "knownCodes lists every warning the registry can produce" <| fun _ -> + // Forgetting to add a code is otherwise silent until somebody writes a directive for + // it and gets told it's a typo - the one thing knownCodes exists to prevent. + let produced = codesTheRegistryCanProduce () + + // Guards against the reflection above quietly finding nothing, which would make every + // assertion below pass for the wrong reason. + if Set.isEmpty produced then + failwith "Found no warnings in WarningCodes - the reflection in this test has gone stale" + + let missing = Set.difference produced WarningCodes.knownCodes + let stale = Set.difference WarningCodes.knownCodes produced + + if not (Set.isEmpty missing) || not (Set.isEmpty stale) then + failwithf "knownCodes is out of sync. Missing: %A. Listed but unused: %A" missing stale + testCase "isSuppressible splits the code bands at FABLE0100" <| fun _ -> // Directive warnings sit below the boundary and can never be silenced by a directive. - WarningCodes.isSuppressible (Some WarningCodes.UnknownSuppressionCode) |> equal false - WarningCodes.isSuppressible (Some WarningCodes.UnusedSuppressionDirective) |> equal false - WarningCodes.isSuppressible (Some WarningCodes.SuppressionBlockWithoutCode) |> equal false + WarningCodes.isSuppressible (Some "FABLE0001") |> equal false + WarningCodes.isSuppressible (Some "FABLE0002") |> equal false + WarningCodes.isSuppressible (Some "FABLE0003") |> equal false WarningCodes.isSuppressible (Some "FABLE0099") |> equal false WarningCodes.isSuppressible (Some "FABLE0100") |> equal true WarningCodes.isSuppressible (Some "FABLE9999") |> equal true diff --git a/tests/Integration/Compiler/IgnoredArgumentTests.fs b/tests/Integration/Compiler/IgnoredArgumentTests.fs index c07d51d347..c1dab678b3 100644 --- a/tests/Integration/Compiler/IgnoredArgumentTests.fs +++ b/tests/Integration/Compiler/IgnoredArgumentTests.fs @@ -67,7 +67,7 @@ open System.Globalization DateTime.Parse("2026-01-01", CultureInfo.InvariantCulture, DateTimeStyles.None) |> ignore """ compile source - |> Assert.Code.noWarning Fable.tran "FABLE0102" + |> Assert.Code.noWarning "FABLE0102" |> Assert.Code.noWarning "FABLE0104" |> ignore From 4c1902888a9eb0db6423a230a7efd41377f17b10 Mon Sep 17 00:00:00 2001 From: Mangel Maxime Date: Fri, 14 Aug 2026 15:23:31 +0200 Subject: [PATCH 7/7] feat: start porting warnings to the new system --- src/Fable.Transforms/Dart/Replacements.fs | 5 +- src/Fable.Transforms/Global/WarningCodes.fs | 39 +++++++--- src/Fable.Transforms/Python/Replacements.fs | 3 +- src/Fable.Transforms/Replacements.fs | 17 ++--- src/Fable.Transforms/Rust/Replacements.fs | 6 +- .../Compiler/IgnoredArgumentTests.fs | 74 +++++++++++++++++++ 6 files changed, 114 insertions(+), 30 deletions(-) diff --git a/src/Fable.Transforms/Dart/Replacements.fs b/src/Fable.Transforms/Dart/Replacements.fs index 0fad7b4587..cf914cfe59 100644 --- a/src/Fable.Transforms/Dart/Replacements.fs +++ b/src/Fable.Transforms/Dart/Replacements.fs @@ -1395,7 +1395,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt | "GetEnumerator", Some c, _ -> stringToCharSeq c |> getEnumerator com r t |> Some | ("Contains" | "StartsWith" | "EndsWith" as meth), Some c, arg :: _ -> if List.isMultiple args then - WarningCodes.secondArgumentIgnored |> addWarningWithCode com ctx.InlinePath r + WarningCodes.stringComparisonIgnored |> addWarningWithCode com ctx.InlinePath r Helper.InstanceCall(c, Naming.lowerFirst meth, t, [ arg ], ?loc = r) |> Some | ReplaceName [ "ToUpper", "toUpperCase" @@ -2714,8 +2714,7 @@ let convert (com: ICompiler) (ctx: Context) r t (i: CallInfo) (_: Expr option) ( | "ToBase64String" | "FromBase64String" -> if not (List.isSingle args) then - $"Convert.%s{Naming.upperFirst i.CompiledName} only accepts one single argument" - |> addWarning com ctx.InlinePath r + WarningCodes.base64ArgumentsIgnored |> addWarningWithCode com ctx.InlinePath r Helper.LibCall( com, diff --git a/src/Fable.Transforms/Global/WarningCodes.fs b/src/Fable.Transforms/Global/WarningCodes.fs index 60fcb8b9d7..885c40dde1 100644 --- a/src/Fable.Transforms/Global/WarningCodes.fs +++ b/src/Fable.Transforms/Global/WarningCodes.fs @@ -44,17 +44,14 @@ let suppressionBlockWithoutCode = /// Used in both the JS/TS and Python replacements. let cultureInfoIgnored = "FABLE0100", "CultureInfo argument is ignored" -/// `String.Contains`/`StartsWith`/`EndsWith` called with an extra argument on the Dart target -/// (a `StringComparison`, a `CultureInfo`, ...): only the comparison itself is honored. Which -/// method it was is not spelled out - the range already points at the call. -let secondArgumentIgnored = - "FABLE0101", "Second argument is ignored: the comparison always uses the target's default rules" - -/// An `IFormatProvider`/`CultureInfo` passed to a `Parse`/`TryParse` overload. The value is -/// always parsed with the invariant rules, so a culture that changes the decimal separator or -/// the day/month order silently changes which value you get. -let formatProviderIgnored = - "FABLE0102", "Format provider argument is ignored, parsing always uses the invariant culture" +/// Dart's `contains`/`startsWith`/`endsWith` are ordinal and case-sensitive, so a +/// `StringComparison` or `CultureInfo` asking for anything else has no effect. +let stringComparisonIgnored = "FABLE0101", "String comparison argument is ignored" + +/// An `IFormatProvider`/`CultureInfo` passed to a `Parse`/`TryParse`, `String.Format` or +/// `StringBuilder.AppendFormat` overload. Fable is culture-independent here, so a culture that +/// changes the decimal separator or the day/month order silently changes the result. +let formatProviderIgnored = "FABLE0102", "Format provider argument is ignored" /// A `NumberStyles` value that isn't `Integer` or `HexNumber` passed to a numeric `Parse`. The /// value is interpolated because the range can't tell you which style was discarded. @@ -64,6 +61,21 @@ let numberStylesIgnored (style: int) = /// A `DateTimeStyles` value passed to a date/time `Parse`. let dateTimeStylesIgnored = "FABLE0104", "DateTimeStyles argument is ignored" +/// A `TimeSpan` constructed with a microseconds argument. The runtime representation only carries +/// milliseconds, so the finer component is dropped rather than rounded. +let timeSpanPrecisionIgnored = + "FABLE0105", "TimeSpan precision is limited to milliseconds, microsecond arguments are ignored" + +/// `FSharpType.IsUnion(t, allowAccessToPrivateRepresentation)` and friends. Fable's reflection +/// has no notion of a private representation, so the flag never restricts anything. +let privateRepresentationFlagIgnored = + "FABLE0106", "Private representation flag is ignored" + +/// `Convert.ToBase64String(bytes, offset, length)` / `(bytes, options)`. Only the array is used, +/// so a slice is encoded whole and line-break options have no effect. +let base64ArgumentsIgnored = + "FABLE0107", "Base64 offset, length and formatting arguments are ignored" + /// Every code the compiler can emit, both bands. A directive naming anything else is reported as /// a typo, so a new warning MUST be added to this list as well as defined above. let knownCodes = @@ -73,10 +85,13 @@ let knownCodes = unusedSuppressionDirective suppressionBlockWithoutCode cultureInfoIgnored - secondArgumentIgnored + stringComparisonIgnored formatProviderIgnored numberStylesIgnored 0 dateTimeStylesIgnored + timeSpanPrecisionIgnored + privateRepresentationFlagIgnored + base64ArgumentsIgnored ] |> List.map fst |> Set.ofList diff --git a/src/Fable.Transforms/Python/Replacements.fs b/src/Fable.Transforms/Python/Replacements.fs index eabe319b65..e74a6455d2 100644 --- a/src/Fable.Transforms/Python/Replacements.fs +++ b/src/Fable.Transforms/Python/Replacements.fs @@ -2933,8 +2933,7 @@ let convert (com: ICompiler) (ctx: Context) r t (i: CallInfo) (_: Expr option) ( | "ToBase64String" | "FromBase64String" -> if not (List.isSingle args) then - $"Convert.%s{Naming.upperFirst i.CompiledName} only accepts one single argument" - |> addWarning com ctx.InlinePath r + WarningCodes.base64ArgumentsIgnored |> addWarningWithCode com ctx.InlinePath r Helper.LibCall(com, "String", (Naming.lowerFirst i.CompiledName), t, args, i.SignatureArgTypes, ?loc = r) |> Some diff --git a/src/Fable.Transforms/Replacements.fs b/src/Fable.Transforms/Replacements.fs index 9f78e52720..0b9dc784f6 100644 --- a/src/Fable.Transforms/Replacements.fs +++ b/src/Fable.Transforms/Replacements.fs @@ -3205,8 +3205,7 @@ let convert (com: ICompiler) (ctx: Context) r t (i: CallInfo) (_: Expr option) ( | "ToBase64String" | "FromBase64String" -> if not (List.isSingle args) then - $"Convert.%s{Naming.upperFirst i.CompiledName} only accepts one single argument" - |> addWarning com ctx.InlinePath r + WarningCodes.base64ArgumentsIgnored |> addWarningWithCode com ctx.InlinePath r Helper.LibCall(com, "String", (Naming.lowerFirst i.CompiledName), t, args, i.SignatureArgTypes, ?loc = r) |> Some @@ -3471,11 +3470,7 @@ let timeSpans (com: ICompiler) (ctx: Context) r (t: Type) (i: CallInfo) (thisArg let limitArgsCountToMilliseconds (meth: string) (maxArgsCount: int) = if args.Length > maxArgsCount then if isNotDefaultInt64ZeroValue args.[maxArgsCount] then - addWarning - com - ctx.InlinePath - r - "TimeSpan precision is limited to milliseconds, microsecond arguments will be ignored" + WarningCodes.timeSpanPrecisionIgnored |> addWarningWithCode com ctx.InlinePath r args |> List.take maxArgsCount |> timeSpanLibCall meth @@ -4371,6 +4366,10 @@ let fsharpType com (ctx: Context) methName (r: SourceLocation option) t (i: Call // Prevent name clash with FSharpValue.GetRecordFields | "GetRecordFields" -> // Drop the trailing `allowAccessToPrivateRepresentation` flag (no meaning in JS/TS) + if List.length args > 1 then + WarningCodes.privateRepresentationFlagIgnored + |> addWarningWithCode com ctx.InlinePath r + Helper.LibCall(com, "Reflection", "getRecordElements", t, List.truncate 1 args, i.SignatureArgTypes, ?loc = r) |> Some | "GetUnionCases" @@ -4381,8 +4380,8 @@ let fsharpType com (ctx: Context) methName (r: SourceLocation option) t (i: Call | "IsTuple" | "IsFunction" -> if List.length args > 1 then - $"FSharpType.%s{methName}(): second argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.privateRepresentationFlagIgnored + |> addWarningWithCode com ctx.InlinePath r let args = [ List.head args ] diff --git a/src/Fable.Transforms/Rust/Replacements.fs b/src/Fable.Transforms/Rust/Replacements.fs index 6ae137197a..8277b52a20 100644 --- a/src/Fable.Transforms/Rust/Replacements.fs +++ b/src/Fable.Transforms/Rust/Replacements.fs @@ -1482,8 +1482,7 @@ let strings (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr opt match args with | (ExprType String :: _) -> "sprintf!" |> emitFormat com r t args |> Some | (cultureInfo :: restArgs) -> - $"String.Format(): Format provider argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r "sprintf!" |> emitFormat com r t restArgs |> Some | _ -> None @@ -1698,8 +1697,7 @@ let stringBuilder (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Ex Helper.LibCall(com, "Util", "sb_Append", t, [ sb; s ], ?loc = r) |> Some | (cultureInfo :: restArgs) -> - $"StringBuilder.AppendFormat(): Format provider argument is ignored" - |> addWarning com ctx.InlinePath r + WarningCodes.formatProviderIgnored |> addWarningWithCode com ctx.InlinePath r let s = "sprintf!" |> emitFormat com None String restArgs diff --git a/tests/Integration/Compiler/IgnoredArgumentTests.fs b/tests/Integration/Compiler/IgnoredArgumentTests.fs index c1dab678b3..a2af15784f 100644 --- a/tests/Integration/Compiler/IgnoredArgumentTests.fs +++ b/tests/Integration/Compiler/IgnoredArgumentTests.fs @@ -126,4 +126,78 @@ Double.Parse("1.5", NumberStyles.Currency, CultureInfo.InvariantCulture) |> igno compile source |> Assert.Code.warning "FABLE0103" |> ignore + + testCase "A TimeSpan built with microseconds is reported" <| fun _ -> + // The runtime representation only carries milliseconds, so the finer component is dropped. + let source = + """ +open System +TimeSpan.FromMilliseconds(1L, 500L) |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0105" + |> ignore + + testCase "A TimeSpan with no microseconds is silent" <| fun _ -> + let source = + """ +open System +TimeSpan.FromMilliseconds(1L, 0L) |> ignore +""" + compile source + |> Assert.Code.noWarning "FABLE0105" + |> ignore + + testCase "A private-representation flag on FSharpType is reported" <| fun _ -> + let source = + """ +open Microsoft.FSharp.Reflection +FSharpType.IsUnion(typeof, true) |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0106" + |> ignore + + testCase "A private-representation flag on GetRecordFields is reported" <| fun _ -> + // This one dropped the flag silently while its seven siblings warned, all in one function. + let source = + """ +open Microsoft.FSharp.Reflection +type R = { A: int } +FSharpType.GetRecordFields(typeof, true) |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0106" + |> ignore + + testCase "FSharpType without the flag is silent" <| fun _ -> + let source = + """ +open Microsoft.FSharp.Reflection +FSharpType.IsUnion(typeof) |> ignore +""" + compile source + |> Assert.Code.noWarning "FABLE0106" + |> ignore + + testCase "Base64 offset and length arguments are reported" <| fun _ -> + // The slice is silently encoded whole, so this is a wrong value rather than a formatting nit. + let source = + """ +open System +Convert.ToBase64String([| 1uy; 2uy; 3uy; 4uy |], 0, 2) |> ignore +""" + compile source + |> Assert.Code.warning "FABLE0107" + |> ignore + + testCase "Base64 with only the array is silent" <| fun _ -> + let source = + """ +open System +Convert.ToBase64String([| 1uy; 2uy; 3uy; 4uy |]) |> ignore +""" + compile source + |> Assert.Code.noWarning "FABLE0107" + |> ignore ]