Skip to content

Commit

Permalink
Prevent AnonymousKernelCommands from ending up inside serialized Kern…
Browse files Browse the repository at this point in the history 
…elEvents.

AnonymousKernelCommands are not supposed to be serialized across process boundaries and should therefore not be included as part of KernelEvents.
  • Loading branch information
@shyamnamboodiripad
shyamnamboodiripad committed Apr 6, 2023
1 parent 9ca2fc3 commit 5d4aef5
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 20 deletions.
20 changes: 18 additions & 2 deletions src/Microsoft.DotNet.Interactive.Tests/Parsing/SubmissionParserTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using FluentAssertions.Execution;
using Microsoft.DotNet.Interactive.Commands;
using Microsoft.DotNet.Interactive.CSharp;
using Microsoft.DotNet.Interactive.Events;
using Microsoft.DotNet.Interactive.FSharp;
using Microsoft.DotNet.Interactive.Jupyter;
using Microsoft.DotNet.Interactive.Parsing;
Expand Down Expand Up @@ -337,7 +338,7 @@ public void root_node_span_always_expands_with_child_nodes()
.Should()
.AllSatisfy(child => rootSpan.Contains(child.Span).Should().BeTrue());
}

private static SubmissionParser CreateSubmissionParser(string defaultLanguage = "csharp")
{
using var compositeKernel = new CompositeKernel
Expand Down Expand Up @@ -365,6 +366,21 @@ private static SubmissionParser CreateSubmissionParser(string defaultLanguage =
return compositeKernel.SubmissionParser;
}

[Fact]
public async Task DiagnosticsProduced_events_always_point_back_to_the_original_command()
{
using var kernel = new CompositeKernel
{
new CSharpKernel().UseValueSharing(),
new FSharpKernel().UseValueSharing(),
};
kernel.DefaultKernelName = "csharp";

var command = new SubmitCode("#!unrecognized");
var result = await kernel.SendAsync(command);
result.Events.Should().ContainSingle<DiagnosticsProduced>().Which.Command.Should().BeSameAs(command);
}

[Fact]
public async Task ParsedDirectives_With_Args_Consume_Newlines()
{
Expand Down Expand Up @@ -447,7 +463,7 @@ public void RequestDiagnostics_can_be_split_into_separate_commands()
// language-specific code";

MarkupTestFile.GetLineAndColumn(markupCode, out var code, out var _, out var _);

var command = new RequestDiagnostics(code);
var commands = new CSharpKernel().UseDefaultMagicCommands().SubmissionParser.SplitSubmission(command);

Expand Down
1 change: 0 additions & 1 deletion src/Microsoft.DotNet.Interactive/Connection/ProxyKernel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ internal override Task HandleAsync(
switch (command)
{
case AnonymousKernelCommand:
return base.HandleAsync(command, context);
case DirectiveCommand:
return base.HandleAsync(command, context);
}
Expand Down
12 changes: 11 additions & 1 deletion src/Microsoft.DotNet.Interactive/Events/KernelEvent.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,17 @@ public abstract class KernelEvent
{
protected KernelEvent(KernelCommand command)
{
Command = command ?? throw new ArgumentNullException(nameof(command));
if (command is null)
{
throw new ArgumentNullException(nameof(command));
}

if (command is AnonymousKernelCommand)
{
throw new ArgumentException($"{nameof(command)} should not be an {nameof(AnonymousKernelCommand)}");
}

Command = command;
RoutingSlip = new EventRoutingSlip();
}

Expand Down
30 changes: 14 additions & 16 deletions src/Microsoft.DotNet.Interactive/Parsing/SubmissionParser.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ public IReadOnlyList<KernelCommand> SplitSubmission(RequestDiagnostics requestDi
requestDiagnostics,
requestDiagnostics.Code,
(languageNode, parent, _) => new RequestDiagnostics(languageNode, parent));
return commands.Where(c => c is RequestDiagnostics ).ToList();

return commands.Where(c => c is RequestDiagnostics).ToList();
}

private delegate KernelCommand CreateChildCommand(
Expand All @@ -88,7 +88,7 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
var nodes = tree.GetRoot().ChildNodes.ToArray();

var targetKernelName = originalCommand.TargetKernelName ?? DefaultKernelName();

var lastCommandScope = originalCommand.SchedulingScope;
KernelNameDirectiveNode lastKernelNameNode = null;

Expand All @@ -97,11 +97,11 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
switch (node)
{
case DirectiveNode directiveNode:
if (KernelInvocationContext.Current is {} context)
if (KernelInvocationContext.Current is { } context)
{
context.CurrentlyParsingDirectiveNode = directiveNode;
}

var parseResult = directiveNode.GetDirectiveParseResult();

if (parseResult.Errors.Any())
Expand All @@ -124,7 +124,7 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
CodeAnalysis.DiagnosticSeverity.Error,
"NI0001", // QUESTION: (SplitSubmission) what code should this be?
"Unrecognized magic command");
var diagnosticsProduced = new DiagnosticsProduced(new[] { diagnostic }, c);
var diagnosticsProduced = new DiagnosticsProduced(new[] { diagnostic }, originalCommand);
context.Publish(diagnosticsProduced);
return Task.CompletedTask;
});
Expand All @@ -141,7 +141,7 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
{
commands.Clear();

if (sendExtraDiagnostics is {})
if (sendExtraDiagnostics is { })
{
commands.Add(sendExtraDiagnostics);
}
Expand Down Expand Up @@ -214,7 +214,6 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
break;

case LanguageNode languageNode:
{
if (commands.Count > 0 &&
commands[^1] is SubmitCode previous)
{
Expand All @@ -230,7 +229,6 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
commands.Add(command);
}
}
}
break;

default:
Expand Down Expand Up @@ -258,7 +256,7 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
if (NoSplitWasNeeded())
{
originalCommand.TargetKernelName ??= targetKernelName;
return new []{originalCommand};
return new[] { originalCommand };
}

foreach (var command in commands)
Expand Down Expand Up @@ -291,8 +289,8 @@ bool NoSplitWasNeeded()
}
}

if (commands.All(c => c.GetType() == originalCommand.GetType() &&
(c.TargetKernelName == originalCommand.TargetKernelName
if (commands.All(c => c.GetType() == originalCommand.GetType() &&
(c.TargetKernelName == originalCommand.TargetKernelName
|| c.TargetKernelName == commands[0].TargetKernelName)))
{
return true;
Expand Down Expand Up @@ -324,7 +322,7 @@ private string DefaultKernelName()
CompositeKernel c => c.DefaultKernelName,
_ => _kernel.Name
};

return kernelName;
}

Expand All @@ -335,7 +333,7 @@ private string DefaultKernelName()
return null;
}

var dict = new Dictionary<string, (SchedulingScope , Func<Parser>)>();
var dict = new Dictionary<string, (SchedulingScope, Func<Parser>)>();

foreach (var childKernel in compositeKernel.ChildKernels)
{
Expand All @@ -347,7 +345,7 @@ private string DefaultKernelName()
}
}

(SchedulingScope, Func<Parser>) GetParser() => (childKernel.SchedulingScope,() => childKernel.SubmissionParser.GetDirectiveParser());
(SchedulingScope, Func<Parser>) GetParser() => (childKernel.SchedulingScope, () => childKernel.SubmissionParser.GetDirectiveParser());
}

return dict;
Expand All @@ -374,7 +372,7 @@ internal Parser GetDirectiveParser()
typeof(KernelInvocationContext),
_ => KernelInvocationContext.Current);
});

_directiveParser = commandLineBuilder.Build();
}

Expand Down

0 comments on commit 5d4aef5

Please sign in to comment.