-
Notifications
You must be signed in to change notification settings - Fork 847
Add warning for XML documentation comments not positioned as first non-whitespace on line #18891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
T-Gro
merged 51 commits into
main
from
copilot/fix-920e1d4e-0c13-4c25-8d7b-183dbc295ea6
Jan 13, 2026
Merged
Changes from 1 commit
Commits
Show all changes
51 commits
Select commit
Hold shift + click to select a range
5c1cb81
Initial plan
Copilot 1713662
Add basic XML doc comment position warning - initial implementation
Copilot 8698eef
Add warning for XML documentation comments not positioned as first no…
Copilot 21333cf
Fix XML doc comment position check to analyze actual line content ins…
Copilot 7d2be7e
Address feedback by disabling hardcoded column limit approach tempora…
Copilot 6413c55
Merge branch 'main' into copilot/fix-920e1d4e-0c13-4c25-8d7b-183dbc29…
T-Gro 92f2c02
Address feedback: remove hardcoded column limit, disable warning temp…
Copilot e12e8cc
Implement XML doc comment position check in LexFilter instead of lexe…
Copilot fb42cf8
Merge branch 'main' into copilot/fix-920e1d4e-0c13-4c25-8d7b-183dbc29…
T-Gro 28a6204
Fix type constraint mismatch in FSComp.SR.xmlDocNotFirstOnLine() usage
Copilot 56607de
Fix unused variable warning in LexFilter pattern match
Copilot 9d9a664
Merge branch 'main' into copilot/fix-920e1d4e-0c13-4c25-8d7b-183dbc29…
T-Gro 83a3454
Update xlf localization files for xmlDocNotFirstOnLine entry
Copilot bd22c0e
Add lexer-level tracking for XML doc comment position warning
Copilot 3070e3d
Fix incorrect /// comment usage in infos.fs - should be //
Copilot 1f45f17
Disable XML doc position warning - implementation too aggressive
Copilot 4ac9a26
Re-implement XML doc comment position warning using token position tr…
Copilot a560672
Add token position tracking to more token types and start updating tests
Copilot 3f936f2
Update XmlDocTests to expect FS3879 warning for misplaced /// comments
Copilot df81dfb
Fix remaining test failures: update neg45.bsl baseline and type membe…
Copilot fc51e55
Fix code formatting in LexHelpers.fs and LexHelpers.fsi
Copilot 4b58239
Merge branch 'main' into copilot/fix-920e1d4e-0c13-4c25-8d7b-183dbc29…
abonie c24643d
Update neg45.bsl baseline with all three FS3879 warnings
Copilot c2736dc
Fix /// comment in Trimming/Program.fs that was causing build failures
Copilot e171853
Change triple slash to regular comments
abonie b4a85cb
Fix XML documentation comment parse errors
abonie b3984a0
Undo changes to neg45.bsl
abonie 0c6e957
Suppress FS3879 warning in AOT trimming test projects
Copilot d4b6661
Add release notes entry for FS3879 warning
Copilot c2112c5
Add test for DU cases with /// after case definition
Copilot 2faae14
Fix DU test - use correct lambda signature (fun _ -> ()) instead of (…
Copilot 4d16c5b
Fix DU test expected column positions - CaseB line should be Col 12, …
Copilot 1006511
Rewrite XML doc position warning using LexFilter approach
Copilot 337178d
Merge branch 'main' into copilot/fix-920e1d4e-0c13-4c25-8d7b-183dbc29…
T-Gro 250ed38
Move release notes entry to 10.0.200.md as requested
Copilot 696e8fb
Fix compilation errors - use COMMENT token and fix ODECLEND tuple
Copilot 94f182c
Fix LexFilter code to properly check for XML doc comments
Copilot b3af608
fix build errors
T-Gro 6a8696e
Apply patch from /run fantomas
actions-user c73cc4e
Fix FS3879 warning - move check from LexFilter to lex.fsl
Copilot 001cbda
Allow XML doc comments after opening brace and equals in record types
Copilot 541d143
Create proper GitHub Copilot Skills per documentation
Copilot 5a4f398
Enhance GitHub Copilot Skills per review feedback
Copilot 2fec170
Change FS3879 to informational warning and enable in F# codebase
Copilot 60dd7bb
Fix invalid XML doc comments in Intellisense.fs
Copilot f3164c7
Merge branch 'main' into copilot/fix-920e1d4e-0c13-4c25-8d7b-183dbc29…
T-Gro c144a34
Apply suggestions from code review
T-Gro c014a7a
Merge branch 'main' into copilot/fix-920e1d4e-0c13-4c25-8d7b-183dbc29…
T-Gro 2a31793
Remove no-op match and add tests for XML doc after = and { in express…
Copilot cca22f3
Update tests/FSharp.Compiler.Service.Tests/XmlDocTests.fs
T-Gro 9fa902a
Update tests/FSharp.Compiler.Service.Tests/XmlDocTests.fs
T-Gro File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix FS3879 warning - move check from LexFilter to lex.fsl
Moved XML doc position warning from LexFilter.fs to lex.fsl where we can properly detect /// comments. The warning now correctly triggers when /// appears after code on the same line. Test Results: - Total: 60 tests - Passed: 55 (was 40) - Failed: 0 (was 15) - Skipped: 5 - Time: 6.8 seconds Build: 3m 43.9s, 0 errors Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
- Loading branch information
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| # XML Doc Warning FS3879 - Investigation Hypotheses | ||
|
|
||
| ## Test Failure Summary | ||
| - **Initial State**: Total tests: 60, Passed: 40, Failed: 15, Skipped: 5, Time: 7.1 seconds | ||
| - **After Fix**: Total tests: 60, Passed: 55, Failed: 0, Skipped: 5, Time: 6.8 seconds | ||
|
|
||
| ## Failed Tests (Before Fix) | ||
| 1. `let bindings 03 - 'let in' with attributes after 'let'` | ||
| 2. `types 03 - xml doc after 'and'` | ||
| 3. `exception 02 - attribute after 'exception'` | ||
| 4. `types 05 - attributes after 'type'` | ||
| 5. `let bindings 09 - xml doc after 'and'` | ||
| 6. `type members 04 - property accessors` | ||
| 7. `let bindings 10 - xml doc before/after 'and'` | ||
| 8. `let bindings 01 - allowed positions` | ||
| 9. `types 04 - xml doc before/after 'and'` | ||
| 10. `Discriminated Union - triple slash after case definition should warn` | ||
| 11. `exception 01 - allowed positions` | ||
| 12. `let bindings 07 - attribute after 'let'` | ||
| 13. `type members 06 - implicit ctor` | ||
| 14. `module 02 - attributes after 'module'` | ||
| 15. `type members 02` | ||
|
|
||
| ## Hypothesis 1: The XmlDocCollector members are not being set correctly | ||
| **Problem**: The `SetLastNonCommentTokenLine` and `GetLastNonCommentTokenLine` methods in LexerStore.fs reference XmlDocCollector members that don't exist or aren't accessible. | ||
|
|
||
| **Evidence**: Compilation errors showed `XmlDocCollector` does not define these members even though they were supposedly added in commit 1006511. | ||
|
|
||
| **Test**: Check if XmlDoc.fs actually has the required members and if they're in the correct scope. | ||
|
|
||
| **Status**: ✅ RESOLVED - The members existed and were accessible. | ||
|
|
||
| ## Hypothesis 2: The warning check in LexFilter is not correctly identifying /// comments | ||
| **Problem**: The code in rulesForBothSoftWhiteAndHardWhite may not be correctly checking for `///` comments or may not have access to the lexeme text. | ||
|
|
||
| **Evidence**: Tests are expecting FS3879 warnings but they're not being emitted. The attempt to access `lexbuf.LexBuffer.Lexeme` is incorrect - LexBuffer doesn't have a LexBuffer property. | ||
|
|
||
| **Test**: Verify how to correctly access comment text in LexFilter and check if the LINE_COMMENT token actually contains the text. | ||
|
|
||
| **Status**: ✅ CONFIRMED - This was a contributing factor but not the root cause. | ||
|
|
||
| ## Hypothesis 3: The warning check is in the wrong location (LexFilter instead of lex.fsl) | ||
| **Problem**: The warning is being checked in LexFilter.fs at line 2640, but at that point: | ||
| 1. We don't have reliable access to the lexeme text to check if it's `///` vs `//` | ||
| 2. The LINE_COMMENT token doesn't distinguish between comment types | ||
| 3. `lexbuf.LexemeView` may not be available or reliable in LexFilter | ||
|
|
||
| **Evidence**: | ||
| - The lexer (lex.fsl) at lines 740-746 specifically matches `///` and knows it's an XML doc comment | ||
| - The lexer already calls XmlDocStore functions (AddGrabPointDelayed) | ||
| - The lexer has access to the lexeme range and can easily check lastNonCommentTokenLine | ||
|
|
||
| **Solution**: Move the warning check from LexFilter.fs to lex.fsl, specifically in the `"///"` pattern handler at line 740-746. | ||
|
|
||
| **Status**: ✅ **CONFIRMED - THIS WAS THE ROOT CAUSE** | ||
|
|
||
| ## Final Solution | ||
| 1. ✅ Removed the incorrect warning check from LexFilter.fs (lines 2640-2660) | ||
| 2. ✅ Added warning check in lex.fsl at line 740-746 (the `///` handler): | ||
| ```fsharp | ||
| let lastNonCommentLine = XmlDocStore.GetLastNonCommentTokenLine lexbuf | ||
| if lastNonCommentLine = m.StartLine then | ||
| warning(Error(FSComp.SR.xmlDocNotFirstOnLine(), m)) | ||
| ``` | ||
| 3. ✅ Build command: `./build.sh -c Release` - Time: 3m 43.9s - Errors: 0 | ||
| 4. ✅ Test command: `dotnet test artifacts/bin/FSharp.Compiler.Service.Tests/Release/net10.0/FSharp.Compiler.Service.Tests.dll --filter "FullyQualifiedName~XmlDocTests"` | ||
| 5. ✅ Final Results: **60 total tests, 55 passed, 0 failed, 5 skipped, 6.8 seconds** |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.