fix(scripting): resolve Medium code-review finding (Core.Scripting-004)
DependencyExtractor.VisitInvocationExpression now additionally checks
that the member-access receiver is the identifier "ctx" before treating
a GetTag / SetVirtualTag call as a ScriptContext dependency. This
prevents spurious dependencies when a script defines a local helper type
with a matching method name and calls it as other.GetTag("X"). Test
Ignores_member_access_GetTag_on_non_ctx_receiver added.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -21,11 +21,15 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Scripting;
|
|||||||
/// token.
|
/// token.
|
||||||
/// </para>
|
/// </para>
|
||||||
/// <para>
|
/// <para>
|
||||||
/// Identifier matching is by spelling: the extractor looks for
|
/// Matching is by spelling: the extractor looks for member-access invocations
|
||||||
/// <c>ctx.GetTag(...)</c> / <c>ctx.SetVirtualTag(...)</c> literally. A deliberately
|
/// whose receiver identifier is literally <c>ctx</c> and whose method name is
|
||||||
/// misspelled method call (<c>ctx.GetTagz</c>) is not picked up but will also fail
|
/// <c>GetTag</c> or <c>SetVirtualTag</c>. A deliberately misspelled method call
|
||||||
/// to compile against <see cref="ScriptContext"/>, so there's no way to smuggle a
|
/// (<c>ctx.GetTagz</c>) is not picked up but will also fail to compile against
|
||||||
/// dependency past the extractor while still having a working script.
|
/// <see cref="ScriptContext"/>, so there is no way to smuggle a dependency past the
|
||||||
|
/// extractor while still having a working script. Calls with the same method name on
|
||||||
|
/// a different receiver (<c>other.GetTag("X")</c>) are explicitly ignored so that
|
||||||
|
/// scripts defining local helper types with matching names do not produce spurious
|
||||||
|
/// dependencies. (Core.Scripting-004.)
|
||||||
/// </para>
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
public static class DependencyExtractor
|
public static class DependencyExtractor
|
||||||
@@ -67,10 +71,15 @@ public static class DependencyExtractor
|
|||||||
|
|
||||||
public override void VisitInvocationExpression(InvocationExpressionSyntax node)
|
public override void VisitInvocationExpression(InvocationExpressionSyntax node)
|
||||||
{
|
{
|
||||||
// Only interested in member-access form: ctx.GetTag(...) / ctx.SetVirtualTag(...).
|
// Only interested in ctx.GetTag(...) / ctx.SetVirtualTag(...) — member-access
|
||||||
// Anything else (free functions, chained calls, static calls) is ignored — but
|
// form where the receiver is the identifier "ctx" (the ScriptGlobals<T>.ctx
|
||||||
// still visit children in case a ctx.GetTag call is nested inside.
|
// field). Calls with the same method name on a different receiver (e.g.
|
||||||
if (node.Expression is MemberAccessExpressionSyntax member)
|
// someHelper.GetTag("X")) are ignored — not picking them up avoids spurious
|
||||||
|
// dependencies when scripts define local types with matching method names.
|
||||||
|
// (Core.Scripting-004.)
|
||||||
|
if (node.Expression is MemberAccessExpressionSyntax member
|
||||||
|
&& member.Expression is IdentifierNameSyntax receiver
|
||||||
|
&& receiver.Identifier.ValueText == "ctx")
|
||||||
{
|
{
|
||||||
var methodName = member.Name.Identifier.ValueText;
|
var methodName = member.Name.Identifier.ValueText;
|
||||||
if (methodName is nameof(ScriptContext.GetTag) or nameof(ScriptContext.SetVirtualTag))
|
if (methodName is nameof(ScriptContext.GetTag) or nameof(ScriptContext.SetVirtualTag))
|
||||||
|
|||||||
@@ -133,7 +133,7 @@ public sealed class DependencyExtractorTests
|
|||||||
}
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void Ignores_non_ctx_method_named_GetTag()
|
public void Ignores_non_ctx_method_named_GetTag_free_function()
|
||||||
{
|
{
|
||||||
// Scripts are free to define their own helper called "GetTag" — as long as it's
|
// Scripts are free to define their own helper called "GetTag" — as long as it's
|
||||||
// not on the ctx instance, the extractor doesn't pick it up. The sandbox
|
// not on the ctx instance, the extractor doesn't pick it up. The sandbox
|
||||||
@@ -147,6 +147,24 @@ public sealed class DependencyExtractorTests
|
|||||||
result.Reads.ShouldBeEmpty();
|
result.Reads.ShouldBeEmpty();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Ignores_member_access_GetTag_on_non_ctx_receiver()
|
||||||
|
{
|
||||||
|
// A member-access call to GetTag on a non-ctx identifier must NOT be treated as
|
||||||
|
// a ScriptContext dependency. The old walker accepted any receiver; the fix
|
||||||
|
// requires the receiver to be the identifier "ctx". (Core.Scripting-004.)
|
||||||
|
var result = DependencyExtractor.Extract(
|
||||||
|
"""
|
||||||
|
class Helper { public object GetTag(string p) => p; }
|
||||||
|
var h = new Helper();
|
||||||
|
var v = h.GetTag("X");
|
||||||
|
return ctx.GetTag("RealTag").Value;
|
||||||
|
""");
|
||||||
|
result.IsValid.ShouldBeTrue();
|
||||||
|
result.Reads.ShouldContain("RealTag");
|
||||||
|
result.Reads.ShouldNotContain("X");
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void Empty_source_is_a_no_op()
|
public void Empty_source_is_a_no_op()
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user