Skip to content

feat(all): support suppressing warnings via // fable-disable comments#4808

Draft
MangelMaxime wants to merge 1 commit into
mainfrom
feat-suppress-fable-warnings
Draft

feat(all): support suppressing warnings via // fable-disable comments#4808
MangelMaxime wants to merge 1 commit into
mainfrom
feat-suppress-fable-warnings

Conversation

@MangelMaxime

Copy link
Copy Markdown
Member

Hello,

In Fable we added support for TreatWarningsAsErrors: some warnings Fable generates as informational signals can be treated as errors when this is enabled.

With this PR, I'm adding support for ESLint-style suppression comments (we can't reuse #nowarn - it only accepts numeric codes and is resolved by FCS before Fable's own warnings even exist - and a new compiler-level directive would mean patching FCS itself).

This adds support for:

  • Disabling the current line: // fable-disable-line
  • Disabling the next line: // fable-disable-next-line
  • Disabling a block: // fable-disable ... // fable-enable

For example, a user recently requested a warning for duplicate CompiledNames on union cases. I originally thought this should be an error, but some libraries (e.g. Fable.React) use duplicate names on purpose to represent JS binding "overloads" - so it could only ever be a warning.

By adding suppression comments, people can keep TreatWarningsAsErrors enabled for safety, while still deciding, case by case, to suppress the specific warnings they judge harmless.


The current codes in src/Fable.Transforms/Global/WarningCodes.fs are here to serve as an example.

@MangelMaxime
MangelMaxime requested review from dbrattli and ncave July 16, 2026 19:26
@MangelMaxime MangelMaxime changed the title feat(all): support suppressing warnings via // fable-disable comments feat(all): support suppressing warnings via // fable-disable comments Jul 16, 2026
/// 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"

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
@github-actions

Copy link
Copy Markdown
Contributor

Python Type Checking Results (Pyright)

Metric Value
Total errors 34
Files with errors 4
Excluded files 4
New errors ✅ No
Excluded files with errors (4 files)

These files have known type errors and are excluded from CI. Remove from pyrightconfig.ci.json as errors are fixed.

File Errors Status
temp/tests/Python/test_hash_set.py 18 Excluded
temp/tests/Python/test_applicative.py 12 Excluded
temp/tests/Python/test_nested_and_recursive_pattern.py 2 Excluded
temp/tests/Python/fable_modules/thoth_json_python/encode.py 2 Excluded

@MangelMaxime
MangelMaxime force-pushed the feat-suppress-fable-warnings branch from 21a98ea to 98286e7 Compare July 22, 2026 20:04

@ncave ncave left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MangelMaxime Looks good, just some thoughts in no particular order (non-blocking, feel free to disagree or ignore):

  • Since files can be compiled in parallel, would the non-thread safe warnings cache work as expected?
  • I may be missing something, but almost all Directive union cases look the same, what's the difference between them, and can they be simplified?
  • It's nice to standardize the warnings with codes, instead of using free form, absolutely. But that also adds some complexity and maintenance on its own.
  • Now that everyone heavily uses AI, perhaps we can eliminate some classes of warnings by just implementing the missing methods or overrides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants