Skip to content

Show [<Literal>] and constant value for IL fields in Go to Metadata#19922

Open
T-Gro wants to merge 7 commits into
mainfrom
fix/issue-11526
Open

Show [<Literal>] and constant value for IL fields in Go to Metadata#19922
T-Gro wants to merge 7 commits into
mainfrom
fix/issue-11526

Conversation

@T-Gro

@T-Gro T-Gro commented Jun 9, 2026

Copy link
Copy Markdown
Member

Fixes #11526

IL literal (const) fields such as System.Char.MaxValue now render with
[<Literal>] and their constant value in Go to Metadata, instead of
appearing as plain static val declarations.

Copilot and others added 3 commits June 9, 2026 08:13
Tests (RED): add failing Go-to-Metadata tests for IL literal fields (#…
ae6a943 
…11526)

Adds a new MetadataAsTextILField module in
tests/FSharp.Compiler.Service.Tests/Symbols.fs covering the rendering of
IL literal (const) static fields via FSharpEntity.TryGetMetadataText().

Tests added:
- Theory with 5 rows (Int32/Int64/Byte/SByte MaxValue/MinValue) asserting
  the rendered metadata contains [<Literal>] and "= <value>".
- Fact for System.Char.MaxValue asserting [<Literal>] and "=" appear on
  the MaxValue line.
- Negative Fact for System.Math.PI (initonly, non-literal) asserting it
  is NOT tagged [<Literal>] and has no "=".

Current state (RED):
  total: 7, failed: 6, succeeded: 1
- 5 Theory rows + Char.MaxValue Fact fail with
  Assert.Contains() Failure: Sub-string not found
  because NicePrint.layoutILFieldInfo currently drops [<Literal>] and
  the literal value.
- Math.PI passes (guards against regression of normal static val
  rendering).

Sprint 02 will implement the fix in src/Compiler/Checking/NicePrint.fs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix #11526: render IL literal fields with [<Literal>] and value in Go…
2ccf48f 
…-to-Metadata

Note: Sprint 01 test assumptions were partially incorrect:

- Math.PI is actually a literal const in IL (not initonly); replaced with System.String.Empty for the non-literal case.

- The 'first line containing fieldName' finder matched doc-comment lines; narrowed to lines containing 'val <fieldName>:'.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Format, release notes, baseline updates for #11526 fix
f62712a 
fantomas no-op (both touched files are in .fantomasignore); no .bsl baseline drift observed in targeted MetadataAsTextILField run (7/7 passed in 13s).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro T-Gro requested a review from a team as a code owner June 9, 2026 14:35
Update release notes with PR link (#19922)
f4526ad 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev

✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label Jun 9, 2026
Move #11526 release-notes entry to VNEXT (11.0.100)
6fc5dd8 
CI `check_release_notes` requires the entry to live in the VNEXT release-notes path (currently 11.0.100.md), not in the shipped 9.0.300.md release. Move the bullet, no behavior change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro T-Gro requested a review from abonie June 12, 2026 10:08
T-Gro
T-Gro commented Jun 25, 2026
View reviewed changes

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.

Reviewed the IL literal field rendering change in NicePrint.fs and the accompanying tests. The core approach (append the init value and the [<Literal>] attribute, gated on finfo.LiteralValue.IsSome, reusing the existing layoutAttribs/layoutILFieldInit helpers) is sound and consistent with how F# literal vals are already printed. One substantive gap noted inline regarding string/null literal fields.

Comment thread src/Compiler/Checking/NicePrint.fs
let fieldL = staticL ^^ WordL.keywordVal ^^ (nameL |> addColonL) ^^ typL
let isLiteral = finfo.LiteralValue.IsSome
let fieldL =
if isLiteral then fieldL ^^ PrintIL.layoutILFieldInit finfo.LiteralValue

@T-Gro T-Gro Jun 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

PrintIL.layoutILFieldInit (defined around line 439) only handles the Bool, Char, integer and float ILFieldInit cases; ILFieldInit.String and ILFieldInit.Null fall through to _ -> None, which renders = (* value unavailable *).

Because this PR now appends [<Literal>] and the init for every literal IL field (isLiteral = finfo.LiteralValue.IsSome), a const string field (very common across the BCL, e.g. System.Net.Mime.MediaTypeNames) or a null-literal field will now render as e.g.

[<Literal>]
static val Foo: string = (* value unavailable *)

That is both a regression over the previous clean static val Foo: string and misleading, since the value is in fact known.

Suggested fix: handle ILFieldInit.String s (emit the quoted/escaped string literal) and ILFieldInit.Null (emit null) in layoutILFieldInit, and add a string-literal case to the new tests in Symbols.fs, which currently cover only numeric and Char types.

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Jun 25, 2026
abonie
abonie requested changes Jun 25, 2026
View reviewed changes

@abonie abonie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree with the comment about ILFieldInit.String/Null, otherwise PR looks good

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Jun 25, 2026
Render ILFieldInit.String and Null literal fields in Go to Metadata
8b17938 
Handle const string and null-literal IL fields in PrintIL.layoutILFieldInit so they emit the quoted string / null instead of '(* value unavailable *)'. The match is now exhaustive over ILFieldInit. Adds a string-literal test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro

T-Gro commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

Addressed in 8b17938: PrintIL.layoutILFieldInit now handles ILFieldInit.String (emits the quoted string) and ILFieldInit.Null (emits
ull), so const string/null literal fields no longer render as (* value unavailable *). The match is now exhaustive over ILFieldInit (removed the _ -> None fallthrough). Added a string-literal test (RuntimeFeature.PortablePdb) to Symbols.fs. All MetadataAsTextILField tests pass.

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

Reviewers

@abonie abonie abonie requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed

Projects

F# Compiler and Tooling
Status: In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Go to Metadata - static field handling is wrong

2 participants

@T-Gro @abonie