From 83f8e7688e8bd7694f2de7e192650ca413cd50a8 Mon Sep 17 00:00:00 2001 From: Richard Webb Date: Sun, 26 Jul 2026 16:32:02 +0100 Subject: [PATCH 1/4] Add a test for 'MaxLinesInFunction' with comments at the end of the file --- .../Rules/Conventions/SourceLength.fs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs index cd960deec..b76947ac2 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs @@ -75,6 +75,23 @@ let dog x = ()""") Assert.IsFalse this.ErrorsExist + [] + member this.FunctionTooManyLinesWithMultiLineCommentAtEnd() = + this.Parse($""" +module Program + +let dog x = + (* + Foo + Bar + *) + %s{generateNewLines (FunctionLength - 4) 4} + (* + Baz + *) + ()""") + Assert.IsFalse this.ErrorsExist + [] member this.FunctionTooManyLinesWithNestsedMultiLineComment() = this.Parse($""" From 43bb69e9cfa71119e660c991ee7712f2437752ba Mon Sep 17 00:00:00 2001 From: Richard Webb Date: Sun, 2 Aug 2026 01:15:30 +0100 Subject: [PATCH 2/4] Add some unit tests for stripMultilineComments This requires making it a top level function, rather than being nested inside checkSourceLengthRule --- src/FSharpLint.Core/AssemblyInfo.fs | 1 + .../SourceLength/SourceLengthHelper.fs | 44 ++++++------ .../Rules/Conventions/SourceLength.fs | 72 +++++++++++++++++++ 3 files changed, 95 insertions(+), 22 deletions(-) diff --git a/src/FSharpLint.Core/AssemblyInfo.fs b/src/FSharpLint.Core/AssemblyInfo.fs index 464cf1ab6..9d30a1f17 100644 --- a/src/FSharpLint.Core/AssemblyInfo.fs +++ b/src/FSharpLint.Core/AssemblyInfo.fs @@ -3,5 +3,6 @@ module FSharpLint.Core.AssemblyInfo open System.Runtime.CompilerServices [] +[] () \ No newline at end of file diff --git a/src/FSharpLint.Core/Rules/Conventions/SourceLength/SourceLengthHelper.fs b/src/FSharpLint.Core/Rules/Conventions/SourceLength/SourceLengthHelper.fs index 965233303..df5c6b3d0 100644 --- a/src/FSharpLint.Core/Rules/Conventions/SourceLength/SourceLengthHelper.fs +++ b/src/FSharpLint.Core/Rules/Conventions/SourceLength/SourceLengthHelper.fs @@ -35,33 +35,33 @@ let rec private getTopLevelBalancedPairs (toProcess: List (beginIndex, index) :: getTopLevelBalancedPairs tail List.Empty | _::restOfStack -> getTopLevelBalancedPairs tail restOfStack +let internal stripMultilineComments (source: string) = + let markers = + multilineCommentMarkerRegex.Matches source + |> Seq.map (fun markerMatch -> + let index = markerMatch.Index + if source.[index] = '(' then + Begin index + else + End index) + |> Seq.sortBy (function | Begin index -> index | End index -> index) + |> Seq.toList + + match getTopLevelBalancedPairs markers List.Empty with + | [] -> source + | pairs -> + + (StringBuilder(source), pairs) + ||> List.fold + (fun (currSource: StringBuilder) (startIndex, endIndex) -> + currSource.Remove(startIndex, (endIndex + multilineCommentMarkerRegexCaptureGroupLength) - startIndex)) + |> _.ToString() + let checkSourceLengthRule (config:Config) range fileContents errorName (skipRanges: array) = let error name lineCount actual = let errorFormatString = Resources.GetString("RulesSourceLengthError") String.Format(errorFormatString, name, lineCount, actual) - let stripMultilineComments (source: string) = - let markers = - multilineCommentMarkerRegex.Matches source - |> Seq.map (fun markerMatch -> - let index = markerMatch.Index - if source.[index] = '(' then - Begin index - else - End index) - |> Seq.sortBy (function | Begin index -> index | End index -> index) - |> Seq.toList - - match getTopLevelBalancedPairs markers List.Empty with - | [] -> source - | pairs -> - - (StringBuilder(source), pairs) - ||> List.fold - (fun (currSource: StringBuilder) (startIndex, endIndex) -> - currSource.Remove(startIndex, (endIndex + multilineCommentMarkerRegexCaptureGroupLength) - startIndex)) - |> _.ToString() - match tryFindTextOfRange range fileContents with | Some(sourceCode) -> let sourceCode = diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs index b76947ac2..26bfc9e83 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs @@ -431,3 +431,75 @@ module Program let foo = "" exception SomeException of string""") Assert.IsFalse(this.ErrorExistsAt(2, 0)) + +// Tests for 'stripMultilineComments' +// ref https://github.com/fsprojects/FSharpLint/issues/869 +[] +type TestStripMultilineComments() = + + [] + member this.RemoveCommentFromStart() = + let input = """ +let dog x = + (* + Foo + Bar + *) + printf System.String.Empty + ()""" + + let expected = """ +let dog x = + + printf System.String.Empty + ()""" + + let actual = FSharpLint.Rules.Helper.SourceLength.stripMultilineComments input + Assert.AreEqual(expected, actual) + + [] + member this.RemoveCommentFromEnd() = + let input = """ +let dog x = + printf System.String.Empty + (* + Baz + *) + ()""" + + let expected = """ +let dog x = + printf System.String.Empty + + ()""" + + let actual = FSharpLint.Rules.Helper.SourceLength.stripMultilineComments input + Assert.AreEqual(expected, actual) + + [] + member this.RemoveMultipleComments() = + let input = """ +let dog x = + (* + Foo (* baz *) + let (*) = id + Bar + *) + let (*) a b = a + b + printf System.String.Empty + (* + Baz + *) + ()""" + + let expected = """ +let dog x = + + let (*) a b = a + b + printf System.String.Empty + + ()""" + + let actual = FSharpLint.Rules.Helper.SourceLength.stripMultilineComments input + Assert.AreEqual(expected, actual) + From 5249289c10beac75b054f4d48b393ecb5374b313 Mon Sep 17 00:00:00 2001 From: Richard Webb Date: Sun, 2 Aug 2026 01:53:35 +0100 Subject: [PATCH 3/4] Rework stripMultilineComments to remove block comments in reverse order This avoids issues where removing one comment changes the offsets of following comments, including some cases where that can result in ArgumentOutOfRangeException due to string operations running off the end of the string --- .../Conventions/SourceLength/SourceLengthHelper.fs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/FSharpLint.Core/Rules/Conventions/SourceLength/SourceLengthHelper.fs b/src/FSharpLint.Core/Rules/Conventions/SourceLength/SourceLengthHelper.fs index df5c6b3d0..9aa5d7656 100644 --- a/src/FSharpLint.Core/Rules/Conventions/SourceLength/SourceLengthHelper.fs +++ b/src/FSharpLint.Core/Rules/Conventions/SourceLength/SourceLengthHelper.fs @@ -47,13 +47,18 @@ let internal stripMultilineComments (source: string) = |> Seq.sortBy (function | Begin index -> index | End index -> index) |> Seq.toList + // Process block comment removal + // - If no comments, return input as is + // - If one comment, just remove it directly + // - If several comments, remove them all starting from the last, as removing them from the front changes the offsets of later ones match getTopLevelBalancedPairs markers List.Empty with | [] -> source + | [ (startIndex, endIndex) ] -> source.Remove(startIndex, (endIndex + multilineCommentMarkerRegexCaptureGroupLength) - startIndex ) | pairs -> - (StringBuilder(source), pairs) - ||> List.fold - (fun (currSource: StringBuilder) (startIndex, endIndex) -> + (pairs, StringBuilder(source)) + ||> List.foldBack + (fun (startIndex, endIndex)(currSource: StringBuilder) -> currSource.Remove(startIndex, (endIndex + multilineCommentMarkerRegexCaptureGroupLength) - startIndex)) |> _.ToString() From 94cfa743c6fa2aff612d66d9e1995cc57c46d12e Mon Sep 17 00:00:00 2001 From: Richard Webb Date: Fri, 7 Aug 2026 15:54:33 +0100 Subject: [PATCH 4/4] Add an additional unit test for MaxLinesInUnion This previously triggered the exception from https://github.com/fsprojects/FSharpLint/issues/869 --- .../Rules/Conventions/SourceLength.fs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs index 26bfc9e83..d87aac2dc 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs @@ -371,7 +371,20 @@ let UnionLength = 500 [] type TestMaxLinesInUnion() = inherit TestAstNodeRuleBase.TestAstNodeRuleBase(MaxLinesInUnion.rule { Config.MaxLines = UnionLength }) - // TODO: Add tests. + + // Test a Union type with an acceptable number of lines, with inline block comments + // This cased used to trip the exception described in https://github.com/fsprojects/FSharpLint/issues/869 + // but should be fixed now. + [] + member this.UnionNotTooManyLines() = + this.Parse """ +/// Represents a single group of bindings in a class with an implicit constructor +type IncrClassBindingGroup = + | IncrClassBindingGroup of Tast.Binding list * (*isStatic:*) bool* (*recursive:*) bool + | IncrClassDo of Expr * (*isStatic:*) bool +""" + + Assert.IsFalse(this.ErrorExistsAt(4, 5)) [] let RecordLength = 500