[WIP] Add warning for XML documentation comments not positioned as first non-whitespace on line #18891
Conversation
Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
…n-whitespace on line Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
@copilot :
You are correctly getting a build failure for src/FSharp.Core/Query.fs , Query.fs at line 1331.
This is a valid XML doc comment, but your approach is utter rubbish.
It must check for being the first non-whitespace token irrespective on any magic hausnumero, you cannot just put a clumsy > 16 in there!
Analyze available data in the lexbuf LexBuffer and make it work without hardcoding a magic number.
Have a look at where LexerStore.SaveXmlDocLine is called, this should give you a hint perharps.
github-project-automation
bot
moved this from New
to In Progress
in F# Compiler and Tooling
…tead of using hardcoded column limit Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Fixed the hardcoded column limit approach. Now properly analyzes the actual line content by examining the lexbuf buffer from start of line to the /// token position, checking for any non-whitespace characters. The Query.fs line 1331 issue is resolved - that properly indented XML doc comment is no longer flagged. The remaining build errors are legitimate issues where /// appears after code on the same line. Commit: 21333cf |
…rily Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
…orarily pending proper line analysis implementation Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
❗ Release notes requiredCaution No release notes found for the changed paths (see table below). Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format. The following format is recommended for this repository:
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request. You can open this PR in browser to add release notes: open in github.dev
|
|
@copilot : Drop your approach, this is not the right level. |
…r level Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
|
@copilot : /home/vsts/work/1/s/src/Compiler/SyntaxTree/LexFilter.fs(2633,24): error FS1182: The value 'cont' is unused [/home/vsts/work/1/s/src/Compiler/FSharp.Compiler.Service.fsproj::TargetFramework=net10.0] |
Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Fixed unused variable warning by changing |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@copilot There is following error in the PR pipeline: follow the instructions in DEVGUIDE.md to update xlf files. |
Co-authored-by: abonie <20281641+abonie@users.noreply.github.com>
Updated the xlf localization files by running |
src/Compiler/SyntaxTree/LexFilter.fs
Outdated
|
|
||
| // For the initial implementation, let's warn about LINE_COMMENTS that appear | ||
| // immediately after tokens on the same line. This will catch the obvious cases | ||
| // of misplaced /// comments. We may need to refine this later. |
There was a problem hiding this comment.
@copilot Add tests that make sure a regular comments (not the /// xml comments) can still appear after some other tokens on the same line
There was a problem hiding this comment.
Added tests for regular comments. The implementation now properly distinguishes /// XML doc comments from regular // comments by tracking non-whitespace token positions at the lexer level. Tests added:
Regular comment after code should not warnRegular comment with double slash after code should not warnMultiple regular comments after code should not warnFour slash comment after code should not warn
Commit: bd22c0e
- Add lastNonWhitespaceTokenLine field to LexArgs to track token positions - Update IdentifierToken and KeywordOrIdentifierToken to track non-whitespace tokens - Add checkXmlDocLinePosition function to warn when /// appears after code - Update FSComp.txt entry to numbered format (3879) for Error exception compatibility - Add tests for regular comments after code (should NOT warn) - Add tests for XML doc comments after code (should warn) Co-authored-by: abonie <20281641+abonie@users.noreply.github.com>
|
@copilot Current implementation is too eager in emitting the warning - it also warns in case of regular comments following other tokens in a line. Here is a build error to investigate in that context: |
The warning correctly identified a misplaced /// comment that was intended as a regular comment, not XML documentation. Changed to // to fix the build. Co-authored-by: abonie <20281641+abonie@users.noreply.github.com>
The warning is working correctly. The error at | ProvidedField _ -> [] /// GENERIC TYPE PROVIDERSThis Commit: 3070e3d |
///comments, not regular//comments///to//(warning working as intended)Current Status
Fixed build error in
infos.fs. The warning correctly identified a misplaced///comment at line 1522:This
///was intended as a regular explanatory comment, not XML documentation. Changed to//to fix the issue.Key Implementation Details
///triggers warning: Regular//comments after code do NOT trigger the warning///Tests
Regular comment after code should not warn- Verifies//comments work normallyRegular comment with double slash after code should not warn- Multiple regular commentsMultiple regular comments after code should not warn- In module contextFour slash comment after code should not warn-////comments work normally///after code triggering warnings💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.