diff --git a/src/Verso.FSharp/Helpers/DiagnosticMapper.cs b/src/Verso.FSharp/Helpers/DiagnosticMapper.cs index 0c1f3112..d4c49fe9 100644 --- a/src/Verso.FSharp/Helpers/DiagnosticMapper.cs +++ b/src/Verso.FSharp/Helpers/DiagnosticMapper.cs @@ -61,7 +61,7 @@ internal static class DiagnosticMapper // FCS columns are 0-based — no change needed return new VersoDiagnostic( Severity: severity.Value, - Message: diag.Message, + Message: Kernel.FSharpProjectContext.HideExecutedCellModules(diag.Message), StartLine: cellStartLine, StartColumn: diag.StartColumn, EndLine: cellEndLine, diff --git a/src/Verso.FSharp/Kernel/FSharpKernel.cs b/src/Verso.FSharp/Kernel/FSharpKernel.cs index 4ba2520e..68aaeb6e 100644 --- a/src/Verso.FSharp/Kernel/FSharpKernel.cs +++ b/src/Verso.FSharp/Kernel/FSharpKernel.cs @@ -468,6 +468,10 @@ public async Task> GetCompletionsAsync(string code, in var completions = new List(); foreach (var item in declInfo.Items) { + // The modules the executed cells are checked in are not the user's names. + if (item.NameInList.StartsWith(FSharpProjectContext.ExecutedCellModulePrefix, StringComparison.Ordinal)) + continue; + completions.Add(new Completion( DisplayText: item.NameInList, InsertText: item.NameInCode, @@ -578,7 +582,7 @@ public async Task> GetDiagnosticsAsync(string code) adjustedLine, colAtEnd, lineText, fsharpNames, FSharpTokenTag.Identifier, FSharpOption.None); - var content = FormatToolTip(toolTip); + var content = FSharpProjectContext.HideExecutedCellModules(FormatToolTip(toolTip)); if (string.IsNullOrWhiteSpace(content)) return null; // Calculate cell-local range for the identifier diff --git a/src/Verso.FSharp/Kernel/FSharpProjectContext.cs b/src/Verso.FSharp/Kernel/FSharpProjectContext.cs index 67462850..217e21b8 100644 --- a/src/Verso.FSharp/Kernel/FSharpProjectContext.cs +++ b/src/Verso.FSharp/Kernel/FSharpProjectContext.cs @@ -59,6 +59,14 @@ public void AddReference(string assemblyPath) /// A tuple of (SourceText, PrefixLineCount, FSharpProjectOptions) where PrefixLineCount /// is the number of lines before the current cell code begins. /// + /// + /// FSI compiles each execution as a fragment of its own and opens it for the ones after, so + /// a later execution may define again a type or module an earlier one defined, as a cell does + /// each time it is run again. In one file that is a duplicate definition, so each executed + /// source is wrapped in a module of its own, which is then opened, the way FSI does it. Script + /// directives are lifted out of the module, since they apply to the whole script, and the + /// fragment's top-level opens are repeated after it, since in FSI they stay open. + /// public (string SourceText, int PrefixLineCount, FSharpProjectOptions Options) BuildDocument(string currentCellCode) { var prefixBuilder = new StringBuilder(); @@ -69,10 +77,10 @@ public void AddReference(string assemblyPath) prefixBuilder.AppendLine($"open {ns}"); } - // Add previously executed sources - foreach (var source in _executedSources) + // Add previously executed sources, one fragment each + for (var i = 0; i < _executedSources.Count; i++) { - prefixBuilder.AppendLine(source); + AppendFragment(prefixBuilder, $"{ExecutedCellModulePrefix}{i + 1}", _executedSources[i]); } var prefix = prefixBuilder.ToString(); @@ -90,6 +98,70 @@ public void AddReference(string assemblyPath) return (combinedSource, prefixLineCount, options); } + /// + /// The name every executed cell's module starts with, so completions can leave them out. + /// + public const string ExecutedCellModulePrefix = "VersoExecutedCell"; + + private static readonly System.Text.RegularExpressions.Regex ExecutedCellModuleName = + new($@"\.{ExecutedCellModulePrefix}\d+\b|\b{ExecutedCellModulePrefix}\d+\.", System.Text.RegularExpressions.RegexOptions.CultureInvariant); + + /// + /// Removes the executed cells' module names from compiler text (a tooltip, a diagnostic), so + /// Notebook.VersoExecutedCell1.Shape reads as Notebook.Shape, as it would with + /// the cells checked in one file. + /// + public static string HideExecutedCellModules(string text) => + text.Contains(ExecutedCellModulePrefix, StringComparison.Ordinal) + ? ExecutedCellModuleName.Replace(text, "") + : text; + + private static readonly string[] ScriptDirectives = + { "#r", "#load", "#I", "#nowarn", "#warnon", "#time", "#help", "#quit", "#i" }; + + private static void AppendFragment(StringBuilder builder, string moduleName, string source) + { + var lines = source.Replace("\r\n", "\n").Split('\n'); + var body = new StringBuilder(); + var opens = new List(); + + foreach (var line in lines) + { + if (IsScriptDirective(line)) + { + builder.AppendLine(line); + continue; + } + + if (line.StartsWith("open ", StringComparison.Ordinal)) + opens.Add(line); + + body.Append(" ").AppendLine(line); + } + + builder.Append("module ").Append(moduleName).AppendLine(" ="); + builder.Append(body); + // A fragment of nothing but directives, or a cell whose last line is a comment, still + // needs a body. + builder.AppendLine(" do ()"); + builder.Append("open ").AppendLine(moduleName); + foreach (var open in opens) + builder.AppendLine(open); + } + + private static bool IsScriptDirective(string line) + { + foreach (var directive in ScriptDirectives) + { + if (line.StartsWith(directive, StringComparison.Ordinal) + && (line.Length == directive.Length || char.IsWhiteSpace(line[directive.Length]))) + { + return true; + } + } + return false; + } + /// /// Clears all accumulated state (for kernel restart). /// diff --git a/tests/Verso.FSharp.Tests/Kernel/ExecutedCellsTests.cs b/tests/Verso.FSharp.Tests/Kernel/ExecutedCellsTests.cs new file mode 100644 index 00000000..ab6fab11 --- /dev/null +++ b/tests/Verso.FSharp.Tests/Kernel/ExecutedCellsTests.cs @@ -0,0 +1,145 @@ +using Verso.Abstractions; +using Verso.FSharp.Kernel; +using Verso.Testing.Stubs; + +namespace Verso.FSharp.Tests.Kernel; + +/// +/// FSI compiles each execution as a fragment of its own, so a cell may define again what an +/// earlier execution defined, the cell that was itself just run among them. The document the +/// kernel checks a cell in has to allow the same, or a cell that has run reports its own +/// definitions as duplicates. +/// +[TestClass] +public class ExecutedCellsTests +{ + private FSharpKernel _kernel = null!; + private StubExecutionContext _context = null!; + + [TestInitialize] + public async Task Setup() + { + _kernel = new FSharpKernel(); + await _kernel.InitializeAsync(); + _context = new StubExecutionContext(); + } + + [TestCleanup] + public async Task Cleanup() + { + await _kernel.DisposeAsync(); + } + + private static string Describe(IEnumerable diagnostics) => + string.Join("; ", diagnostics.Select(d => $"{d.StartLine}:{d.StartColumn} {d.Code} {d.Message}")); + + [TestMethod] + public async Task RunCell_CheckedAgain_HasNoDuplicateDefinitions() + { + var code = "module Geometry =\n let area r = r * r\ntype Shape = Circle of float\nlet unit = 1.0"; + await _kernel.ExecuteAsync(code, _context); + + var diagnostics = await _kernel.GetDiagnosticsAsync(code); + + Assert.AreEqual(0, diagnostics.Count(d => d.Severity == DiagnosticSeverity.Error), Describe(diagnostics)); + } + + [TestMethod] + public async Task RunCell_RedefinedByAnotherCell_TheLaterDefinitionWins() + { + await _kernel.ExecuteAsync("let value = 1", _context); + await _kernel.ExecuteAsync("let value = \"text\"", _context); + + var diagnostics = await _kernel.GetDiagnosticsAsync("let n : string = value"); + + Assert.AreEqual(0, diagnostics.Count, Describe(diagnostics)); + } + + [TestMethod] + public async Task OpenInARunCell_StaysOpenForLaterCells() + { + await _kernel.ExecuteAsync("open System.Text", _context); + + var diagnostics = await _kernel.GetDiagnosticsAsync("let sb = StringBuilder()"); + + Assert.AreEqual(0, diagnostics.Count, Describe(diagnostics)); + } + + [TestMethod] + public async Task ModuleFromARunCell_CanBeOpenedLater() + { + await _kernel.ExecuteAsync("module Geometry =\n let area r = r * r", _context); + + var diagnostics = await _kernel.GetDiagnosticsAsync("open Geometry\nlet a = area 2.0\nlet b = Geometry.area 3.0"); + + Assert.AreEqual(0, diagnostics.Count, Describe(diagnostics)); + } + + [TestMethod] + public async Task OpenOfAModuleInTheSameRunCell_StaysOpenForLaterCells() + { + await _kernel.ExecuteAsync("module Geometry =\n let area r = r * r\nopen Geometry", _context); + + var diagnostics = await _kernel.GetDiagnosticsAsync("let a = area 2.0"); + + Assert.AreEqual(0, diagnostics.Count, Describe(diagnostics)); + } + + [TestMethod] + public async Task RunCellWithAReference_StillResolvesLater() + { + var dll = typeof(System.Text.Json.JsonSerializer).Assembly.Location; + await _kernel.ExecuteAsync($"#r @\"{dll}\"\nlet options = System.Text.Json.JsonSerializerOptions()", _context); + + var diagnostics = await _kernel.GetDiagnosticsAsync("let indented = options.WriteIndented"); + + Assert.AreEqual(0, diagnostics.Count(d => d.Severity == DiagnosticSeverity.Error), Describe(diagnostics)); + } + + [TestMethod] + public async Task RunCellWithTripleQuotedString_StillChecks() + { + await _kernel.ExecuteAsync("let text = \"\"\"\nline one\nline two\"\"\"", _context); + + var diagnostics = await _kernel.GetDiagnosticsAsync("let n = text.Length"); + + Assert.AreEqual(0, diagnostics.Count, Describe(diagnostics)); + } + + [TestMethod] + public async Task Completions_DoNotOfferTheExecutedCellModules() + { + await _kernel.ExecuteAsync("let value = 1", _context); + + var completions = await _kernel.GetCompletionsAsync("let x = valu", 12); + + Assert.IsTrue(completions.Any(c => c.DisplayText == "value"), "the earlier cell's binding"); + Assert.IsFalse(completions.Any(c => c.DisplayText.Contains("Verso", StringComparison.OrdinalIgnoreCase) + && c.Kind == "Module"), + string.Join(", ", completions.Where(c => c.Kind == "Module").Select(c => c.DisplayText))); + } + [TestMethod] + public async Task HoverAndDiagnostics_DoNotNameTheExecutedCellModules() + { + await _kernel.ExecuteAsync("module Geometry =\n let area r = r * r\ntype Shape = Circle of float", _context); + + var code = "let a = Geometry.area 2.0\nlet s : Shape = 1"; + var hover = await _kernel.GetHoverInfoAsync(code, code.IndexOf("Geometry", StringComparison.Ordinal) + 1); + var diagnostics = await _kernel.GetDiagnosticsAsync(code); + + Assert.IsNotNull(hover); + StringAssert.DoesNotMatch(hover.Content, new System.Text.RegularExpressions.Regex(FSharpProjectContext.ExecutedCellModulePrefix)); + Assert.IsTrue(diagnostics.Count > 0, "the int assigned to a Shape"); + foreach (var d in diagnostics) + StringAssert.DoesNotMatch(d.Message, new System.Text.RegularExpressions.Regex(FSharpProjectContext.ExecutedCellModulePrefix)); + } + + [TestMethod] + public void HideExecutedCellModules_KeepsTheRestOfTheName() + { + Assert.AreEqual("module Geometry from Notebook", + FSharpProjectContext.HideExecutedCellModules("module Geometry from Notebook.VersoExecutedCell1")); + Assert.AreEqual("expected 'Geometry.Shape'", + FSharpProjectContext.HideExecutedCellModules("expected 'VersoExecutedCell12.Geometry.Shape'")); + } +}