Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use InterceptorsNamespaces feature name instead of InterceptorsPreviewNamespaces #74865

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Use InterceptorsNamespaces feature name instead of InterceptorsPreviewNamespaces #74865

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3f018d
Use InterceptorsNamespaces feature name instead of InterceptorsPrevie…
RikkiGibson Aug 22, 2024
d2b3756
update diagnostic message
RikkiGibson Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions docs/features/interceptors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,12 +218,14 @@ There is an experimental public API `GetInterceptorMethod(this SemanticModel, In

### User opt-in

To use interceptors, the user project must specify the property `<InterceptorsPreviewNamespaces>`. This is a list of namespaces which are allowed to contain interceptors.
To use interceptors, the user project must specify the property `<InterceptorsNamespaces>`. This is a list of namespaces which are allowed to contain interceptors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InterceptorsNamespaces

Should the name be "InterceptorNamespaces" rather than "Interceptors..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the issue there seemed to be agreement on the name InterceptorsNamespaces, i.e. the namespaces which contain interceptors. So I prefer to keep this name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it feels a bit inconsistent that the "Interceptors" part is plural, I would expect this to be named like InterceptorNamespaces.

```xml
<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);Microsoft.AspNetCore.Http.Generated;MyLibrary.Generated</InterceptorsPreviewNamespaces>
<InterceptorsNamespaces>$(InterceptorsNamespaces);Microsoft.AspNetCore.Http.Generated;MyLibrary.Generated</InterceptorsNamespaces>
```

It's expected that each entry in the `InterceptorsPreviewNamespaces` list roughly corresponds to one source generator. Well-behaved components are expected to not insert interceptors into namespaces they do not own.
It's expected that each entry in the `InterceptorsNamespaces` list roughly corresponds to one source generator. Well-behaved components are expected to not insert interceptors into namespaces they do not own.

For compatibility, the property `<InterceptorsPreviewNamespaces>` can be used as an alias for `<InterceptorsNamespaces>`. If both properties have non-empty values, they will be concatenated together in the order `$(InterceptorsNamespaces);$(InterceptorsPreviewNamespaces)` when passed to the compiler.
Copy link
Member

@cston cston Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concatenated together

Will we avoid duplicate interceptors if the two properties have the same namespaces? (Is there a scenario where both properties are set identically, to handle different builds of the compiler?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that test GetInterceptorMethod_10 shows that we don't behave badly if the same namespace is included multiple times.


### Implementation strategy

Expand Down
12 changes: 6 additions & 6 deletions src/Compilers/CSharp/Portable/CSharpParseOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public sealed class CSharpParseOptions : ParseOptions, IEquatable<CSharpParseOpt
public static CSharpParseOptions Default { get; } = new CSharpParseOptions();

private ImmutableDictionary<string, string> _features;
private ImmutableArray<ImmutableArray<string>> _interceptorsPreviewNamespaces;
private ImmutableArray<ImmutableArray<string>> _interceptorsNamespaces;

/// <summary>
/// Gets the effective language version, which the compiler uses to select the
Expand Down Expand Up @@ -177,21 +177,21 @@ public override IReadOnlyDictionary<string, string> Features
}
}

internal ImmutableArray<ImmutableArray<string>> InterceptorsPreviewNamespaces
internal ImmutableArray<ImmutableArray<string>> InterceptorsNamespaces
{
get
{
if (!_interceptorsPreviewNamespaces.IsDefault)
if (!_interceptorsNamespaces.IsDefault)
{
return _interceptorsPreviewNamespaces;
return _interceptorsNamespaces;
}

// e.g. [["System", "Threading"], ["System", "Collections"]]
ImmutableArray<ImmutableArray<string>> previewNamespaces = Features.TryGetValue("InterceptorsPreviewNamespaces", out var namespaces) && namespaces.Length > 0
ImmutableArray<ImmutableArray<string>> previewNamespaces = Features.TryGetValue("InterceptorsNamespaces", out var namespaces) && namespaces.Length > 0
? makeNamespaces(namespaces)
: ImmutableArray<ImmutableArray<string>>.Empty;

ImmutableInterlocked.InterlockedInitialize(ref _interceptorsPreviewNamespaces, previewNamespaces);
ImmutableInterlocked.InterlockedInitialize(ref _interceptorsNamespaces, previewNamespaces);
return previewNamespaces;

static ImmutableArray<ImmutableArray<string>> makeNamespaces(string namespaces)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ internal sealed override CommandLineArguments CommonParse(IEnumerable<string> ar
}
else
{
// When a features value like "InterceptorsPreviewNamespaces=NS1;NS2" is provided,
// When a features value like "InterceptorsNamespaces=NS1;NS2" is provided,
// the build system will quote it so that splitting doesn't occur in the wrong layer.
// We need to unquote here so that subsequent layers can properly identify the feature name and value.
features.Add(value.Unquote());
Expand Down
8 changes: 4 additions & 4 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,7 +1020,7 @@ private void DecodeInterceptsLocationChecksumBased(DecodeWellKnownAttributeArgum
return;
}

var interceptorsNamespaces = ((CSharpParseOptions)attributeNameSyntax.SyntaxTree.Options).InterceptorsPreviewNamespaces;
var interceptorsNamespaces = ((CSharpParseOptions)attributeNameSyntax.SyntaxTree.Options).InterceptorsNamespaces;
var thisNamespaceNames = getNamespaceNames(this);
var foundAnyMatch = interceptorsNamespaces.Any(static (ns, thisNamespaceNames) => isDeclaredInNamespace(thisNamespaceNames, ns), thisNamespaceNames);
if (!foundAnyMatch)
Expand Down Expand Up @@ -1149,7 +1149,7 @@ static void reportFeatureNotEnabled(BindingDiagnosticBag diagnostics, Location a
}
else
{
var recommendedProperty = $"<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);{string.Join(".", namespaceNames)}</InterceptorsPreviewNamespaces>";
var recommendedProperty = $"<InterceptorsNamespaces>$(InterceptorsNamespaces);{string.Join(".", namespaceNames)}</InterceptorsNamespaces>";
diagnostics.Add(ErrorCode.ERR_InterceptorsFeatureNotEnabled, attributeLocation, recommendedProperty);
}
}
Expand All @@ -1170,7 +1170,7 @@ private void DecodeInterceptsLocationAttributeExperimentalCompat(
const int lineNumberParameterIndex = 1;
const int characterNumberParameterIndex = 2;

var interceptorsNamespaces = ((CSharpParseOptions)attributeSyntax.SyntaxTree.Options).InterceptorsPreviewNamespaces;
var interceptorsNamespaces = ((CSharpParseOptions)attributeSyntax.SyntaxTree.Options).InterceptorsNamespaces;
var thisNamespaceNames = getNamespaceNames();
var foundAnyMatch = interceptorsNamespaces.Any(ns => isDeclaredInNamespace(thisNamespaceNames, ns));
if (!foundAnyMatch)
Expand Down Expand Up @@ -1362,7 +1362,7 @@ static void reportFeatureNotEnabled(BindingDiagnosticBag diagnostics, AttributeS
}
else
{
var recommendedProperty = $"<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);{string.Join(".", namespaceNames)}</InterceptorsPreviewNamespaces>";
var recommendedProperty = $"<InterceptorsNamespaces>$(InterceptorsNamespaces);{string.Join(".", namespaceNames)}</InterceptorsNamespaces>";
diagnostics.Add(ErrorCode.ERR_InterceptorsFeatureNotEnabled, attributeSyntax, recommendedProperty);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/Source/SourceModuleSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ void discoverInterceptors()
var toVisit = ArrayBuilder<NamespaceOrTypeSymbol>.GetInstance();

// Search the namespaces which were indicated to contain interceptors.
ImmutableArray<ImmutableArray<string>> interceptorsNamespaces = ((CSharpParseOptions)location.SourceTree.Options).InterceptorsPreviewNamespaces;
ImmutableArray<ImmutableArray<string>> interceptorsNamespaces = ((CSharpParseOptions)location.SourceTree.Options).InterceptorsNamespaces;
foreach (ImmutableArray<string> namespaceParts in interceptorsNamespaces)
{
if (namespaceParts is ["global"])
Expand Down
45 changes: 33 additions & 12 deletions src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12250,9 +12250,9 @@ public void StrongNameProviderWithCustomTempPath()
}

[Theory]
[InlineData(@"/features:InterceptorsPreviewNamespaces=NS1.NS2;NS3.NS4")]
[InlineData(@"/features:""InterceptorsPreviewNamespaces=NS1.NS2;NS3.NS4""")]
public void FeaturesInterceptorsPreviewNamespaces_OptionParsing(string features)
[InlineData(@"/features:InterceptorsNamespaces=NS1.NS2;NS3.NS4")]
[InlineData(@"/features:""InterceptorsNamespaces=NS1.NS2;NS3.NS4""")]
public void FeaturesInterceptorsNamespaces_OptionParsing(string features)
{
var tempDir = Temp.CreateDirectory();
var workingDir = Temp.CreateDirectory();
Expand All @@ -12263,33 +12263,54 @@ public void FeaturesInterceptorsPreviewNamespaces_OptionParsing(string features)
var comp = (CSharpCompilation)csc.CreateCompilation(new StringWriter(), new TouchedFileLogger(), errorLogger: null);
var options = comp.SyntaxTrees[0].Options;
Assert.Equal(1, options.Features.Count);
Assert.Equal("NS1.NS2;NS3.NS4", options.Features["InterceptorsPreviewNamespaces"]);
Assert.Equal("NS1.NS2;NS3.NS4", options.Features["InterceptorsNamespaces"]);

var previewNamespaces = ((CSharpParseOptions)options).InterceptorsPreviewNamespaces;
var previewNamespaces = ((CSharpParseOptions)options).InterceptorsNamespaces;
Assert.Equal(2, previewNamespaces.Length);
Assert.Equal(new[] { "NS1", "NS2" }, previewNamespaces[0]);
Assert.Equal(new[] { "NS3", "NS4" }, previewNamespaces[1]);
}

[Fact]
public void FeaturesInterceptorsPreviewNamespaces_Duplicate()
public void FeaturesInterceptorsNamespaces_Duplicate()
{
var tempDir = Temp.CreateDirectory();
var workingDir = Temp.CreateDirectory();
workingDir.CreateFile("a.cs");

var buildPaths = new BuildPaths(clientDir: "", workingDir: workingDir.Path, sdkDir: null, tempDir: tempDir.Path);
var csc = new MockCSharpCompiler(null, buildPaths, args: new[] { @"/features:InterceptorsPreviewNamespaces=NS1.NS2", @"/features:InterceptorsPreviewNamespaces=NS3.NS4", "a.cs" });
var csc = new MockCSharpCompiler(null, buildPaths, args: new[] { @"/features:InterceptorsNamespaces=NS1.NS2", @"/features:InterceptorsNamespaces=NS3.NS4", "a.cs" });
var comp = (CSharpCompilation)csc.CreateCompilation(new StringWriter(), new TouchedFileLogger(), errorLogger: null);
var options = comp.SyntaxTrees[0].Options;
Assert.Equal(1, options.Features.Count);
Assert.Equal("NS3.NS4", options.Features["InterceptorsPreviewNamespaces"]);
Assert.Equal("NS3.NS4", options.Features["InterceptorsNamespaces"]);

var previewNamespaces = ((CSharpParseOptions)options).InterceptorsPreviewNamespaces;
var previewNamespaces = ((CSharpParseOptions)options).InterceptorsNamespaces;
Assert.Equal(1, previewNamespaces.Length);
Assert.Equal(new[] { "NS3", "NS4" }, previewNamespaces[0]);
}

[Fact]
public void FeaturesInterceptorsPreviewNamespaces_NotRecognizedInCommandLine()
{
// '<InterceptorsPreviewNamespaces>' is recognized in the build task and passed through as a '/features:InterceptorsNamespaces=...' argument.
// '/features:InterceptorsPreviewNamespaces=...' is included in the Features dictionary but does not enable the interceptors feature.
var tempDir = Temp.CreateDirectory();
var workingDir = Temp.CreateDirectory();
workingDir.CreateFile("a.cs");

var buildPaths = new BuildPaths(clientDir: "", workingDir: workingDir.Path, sdkDir: null, tempDir: tempDir.Path);
var csc = new MockCSharpCompiler(null, buildPaths, args: new[] { @"/features:InterceptorsPreviewNamespaces=NS1.NS2", "a.cs" });
var comp = (CSharpCompilation)csc.CreateCompilation(new StringWriter(), new TouchedFileLogger(), errorLogger: null);
var options = comp.SyntaxTrees[0].Options;

Assert.Equal(1, options.Features.Count);
Assert.Equal("NS1.NS2", options.Features["InterceptorsPreviewNamespaces"]);

Assert.False(options.Features.ContainsKey("InterceptorsNamespaces"));
Assert.Empty(((CSharpParseOptions)options).InterceptorsNamespaces);
}

public class QuotedArgumentTests : CommandLineTestBase
{
private static readonly string s_rootPath = ExecutionConditionUtil.IsWindows
Expand Down Expand Up @@ -14373,7 +14394,7 @@ public InterceptsLocationAttribute(string filePath, int line, int character)
""";
var generator = new SingleFileTestGenerator(generatedSource, "Generated.cs");

VerifyOutput(dir, src, includeCurrentAssemblyAsAnalyzerReference: false, additionalFlags: ["/langversion:preview", "/out:embed.exe", "/features:InterceptorsPreviewNamespaces=Generated"], generators: [generator], analyzers: null);
VerifyOutput(dir, src, includeCurrentAssemblyAsAnalyzerReference: false, additionalFlags: ["/langversion:preview", "/out:embed.exe", "/features:InterceptorsNamespaces=Generated"], generators: [generator], analyzers: null);
ValidateWrittenSources([]);

// Clean up temp files
Expand Down Expand Up @@ -14432,7 +14453,7 @@ public InterceptsLocationAttribute(string filePath, int line, int character)
var generator = new SingleFileTestGenerator(generatedSource, "Generated.cs");
var objDir = dir.CreateDirectory("obj");

VerifyOutput(dir, src, includeCurrentAssemblyAsAnalyzerReference: false, additionalFlags: ["/langversion:preview", $"/out:{objDir.Path}/embed.exe", "/features:InterceptorsPreviewNamespaces=Generated"], generators: [generator], analyzers: null);
VerifyOutput(dir, src, includeCurrentAssemblyAsAnalyzerReference: false, additionalFlags: ["/langversion:preview", $"/out:{objDir.Path}/embed.exe", "/features:InterceptorsNamespaces=Generated"], generators: [generator], analyzers: null);
ValidateWrittenSources([]);

// Clean up temp files
Expand Down Expand Up @@ -14502,7 +14523,7 @@ public InterceptsLocationAttribute(string filePath, int line, int character)
"/generatedfilesout:" + objDir.Path,
"/langversion:preview",
"/out:embed.exe",
"/features:InterceptorsPreviewNamespaces=Generated",
"/features:InterceptorsNamespaces=Generated",
.. string.IsNullOrEmpty(pathMapArgument) ? default(Span<string>) : [pathMapArgument]
],
generators: [generator],
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ public static void M()
""", $"C{i}.cs"));
}

var verifier = CompileAndVerify(files.ToArrayAndFree(), parseOptions: TestOptions.Regular.WithFeature("InterceptorsPreviewNamespaces", "global"), expectedOutput: makeExpectedOutput());
var verifier = CompileAndVerify(files.ToArrayAndFree(), parseOptions: TestOptions.Regular.WithFeature("InterceptorsNamespaces", "global"), expectedOutput: makeExpectedOutput());
verifier.VerifyDiagnostics();

string makeExpectedOutput()
Expand Down
Loading