Validate generic arguments in `using static` directives #30737

RikkiGibson · 2018-10-24T23:01:13Z

Resolves #30726

Looks like there is a separate Validate() phase in Imports which takes place after binding, probably because attempting certain validations during binding can result in a stack overflow.

This PR adds to the Imports.Validate method to validate constraints on using static. It was already validating constraints on using aliases which explains what we were seeing before @AlekseyTs.

RikkiGibson · 2018-10-24T23:18:43Z

src/Compilers/CSharp/Test/Semantic/Semantics/BindingTests.cs

+        public void UsingStaticGenericConstraint()
+        {
+            var code = @"
+using static Test<System.String>;


Note that the diagnostic we produce will squiggle all of Test<System.String> instead of just System.String. This is maybe not perfect (why not squiggle the particular type argument that's wrong?) but shouldn't impact usability much.

RikkiGibson · 2018-10-24T23:31:07Z

src/Compilers/CSharp/Portable/Binder/Imports.cs

+                    var typeSymbol = (TypeSymbol)@using.NamespaceOrType;
+                    var corLibrary = typeSymbol.ContainingAssembly.CorLibrary;
+                    var conversions = new TypeConversions(corLibrary);
+                    typeSymbol.CheckAllConstraints(conversions, @using.UsingDirective.Name.Location, semanticDiagnostics);


Looks like there's an NRE in here.

It looks like UsingDirective can be null in scripting scenarios? It seems like a location is always necessary in order to provide diagnostics. Not sure what to do here when @using.UsingDirective is null.

It looks like UsingDirective can be null in scripting scenarios? Do I need to supply an alternate location in such cases?

We usually use NoLocation.Singleton in situations when there is no specific syntax to refer to.

In reply to: 227996503 [](ancestors = 227996503)

jaredpar · 2018-10-24T23:31:56Z

src/Compilers/CSharp/Portable/Binder/Imports.cs

+            foreach (var @using in Usings)
+            {
+                // Check if `using static` directives meet constraints.
+                if (@using.NamespaceOrType.IsType)


Is it worth explicitly checking if this is a static using? Today yes when NamespaceOrType.IsType is true then it's a using static. That's possibly something that is not going to be true in the future though. Think it might benefit from a more explicit check. Possibly add a property to NamespaceOrTypeUsingDirective called IsStatic that does this under the hood.

Is it worth explicitly checking if this is a static using?

It feels like it shouldn't matter whether the using is static or not, the constraints should be satisfied anyway.

In reply to: 227993769 [](ancestors = 227993769)

Not certain, but I'm leaning toward no because it seems like any conceivable feature that involves referencing a type in a using directive should still have the compiler check constraints on that type.

Sounds good.

AlekseyTs · 2018-10-24T23:42:02Z

src/Compilers/CSharp/Portable/Binder/Imports.cs

+                if (@using.NamespaceOrType.IsType)
+                {
+                    var typeSymbol = (TypeSymbol)@using.NamespaceOrType;
+                    var corLibrary = typeSymbol.ContainingAssembly.CorLibrary;


var corLibrary = typeSymbol.ContainingAssembly.CorLibrary; [](start = 20, length = 58)

I think it is more appropriate to request CorLibrary from _compilation.SourceAssembly rather than from the assembly that defines the type. The type name in the using was resolved in context of the _compilation. We will also be able to share a single TypeConversions instance to check all types. #Closed

Addressed in 2fb9e18

cston · 2018-10-25T01:10:11Z

src/Compilers/CSharp/Test/Semantic/Semantics/BindingTests.cs

+                // (2,14): error CS0453: The type 'string' must be a non-nullable value type in order to use it as parameter 'T' in the generic type or method 'Test<T>'
+                // using static Test<System.String>;
+                Diagnostic(ErrorCode.ERR_ValConstraintNotSatisfied, "Test<System.String>").WithArguments("Test<T>", "T", "string").WithLocation(2, 14));
+        }


} [](start = 8, length = 1)

Consider testing constraint errors in containing types and nested types as well.

using static A<A<int>[]>.B; class A<T> where T : class { internal static class B { } }

Done in 53b307e

jaredpar · 2018-10-25T01:17:33Z

src/Compilers/CSharp/Portable/Binder/Imports.cs

+                {
+                    var typeSymbol = (TypeSymbol)@using.NamespaceOrType;
+                    var corLibrary = _compilation.SourceAssembly.CorLibrary;
+                    var conversions = new TypeConversions(corLibrary);


Do we need to recreate conversions for every type or can it be re-used here? Believe it can be re-used as it's the same for every iteration.

You're right. There's no reason to believe it will change per-using.

Note that basically the same conversions object is created in order to check constraints on using aliases. Any benefit in trying to lazy-load it on the compilation or something and reuse it?

Note that basically the same conversions object is created in order to check constraints on using aliases. Any benefit in trying to lazy-load it on the compilation or something and reuse it?

It is probably safe to use Conversions cached on a compilation in all these places

In reply to: 228274243 [](ancestors = 228274243)

RikkiGibson · 2018-10-25T18:09:26Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/BaseClassTests.cs

@@ -2070,8 +2070,11 @@ AbstractClass Test()
 }";
            var comp = CreateCompilation(text);
            comp.VerifyDiagnostics(
+    // (2,14): error CS0311: The type 'object' cannot be used as type parameter 'T' in the generic type or method 'Crash<T>'. There is no implicit reference conversion from 'object' to 'CrashTest.Crash<object>.AbstractClass'.
+    // using static CrashTest.Crash<object>;
+    Diagnostic(ErrorCode.ERR_GenericConstraintNotSatisfiedRefType, "CrashTest.Crash<object>").WithArguments("CrashTest.Crash<T>", "CrashTest.Crash<object>.AbstractClass", "T", "object").WithLocation(2, 14),


This test contains a using static which violates a generic constraint. Before, we emitted a diagnostic on the usage of a class contained within the type imported via using static, but it seems correct to add a diagnostic to the using static directive itself here too.

RikkiGibson · 2018-10-25T19:21:32Z

Determinism test is broken. Weird.

2018-10-25T18:25:05.8119000Z ##[warning]Unable to run "git clean -ffdx" and "git reset --hard HEAD" successfully, delete source folder instead.
2018-10-25T18:25:12.8272801Z ##[error]System.AggregateException: One or more errors occurred. (One or more errors occurred. (Access to the path 'F:\vsagent\9\s\Binaries\Temp\VBCSCompiler\AnalyzerAssemblyLoader\eefc8a0313a344e783a6e8f6c55f7725\9\Microsoft.NetCore.VisualBasic.Analyzers.dll' is denied.)) (Access to the path 'F:\vsagent\9\s\Binaries\Temp\VBCSCompiler\AnalyzerAssemblyLoader\eefc8a0313a344e783a6e8f6c55f7725\9\Microsoft.NetCore.VisualBasic.Analyzers.dll' is denied.) ---> System.UnauthorizedAccessException: Access to the path 'F:\vsagent\9\s\Binaries\Temp\VBCSCompiler\AnalyzerAssemblyLoader\eefc8a0313a344e783a6e8f6c55f7725\9\Microsoft.NetCore.VisualBasic.Analyzers.dll' is denied.
   at System.IO.FileSystem.DeleteFile(String fullPath)
   at System.IO.FileInfo.Delete()
   at Microsoft.VisualStudio.Services.Agent.Util.IOUtil.<>c__DisplayClass9_1.<DeleteDirectory>b__0(FileSystemInfo item)
   at System.Linq.Parallel.ForAllOperator`1.ForAllEnumerator`1.MoveNext(TInput& currentElement, Int32& currentKey)
   at System.Linq.Parallel.ForAllSpoolingTask`2.SpoolingWork()
   at System.Linq.Parallel.SpoolingTaskBase.Work()
   at System.Linq.Parallel.QueryTask.BaseWork(Object unused)
   at System.Linq.Parallel.QueryTask.<>c.<.cctor>b__10_0(Object o)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot)
   --- End of inner exception stack trace ---
   at System.Linq.Parallel.QueryTaskGroupState.QueryEnd(Boolean userInitiatedDispose)
   at System.Linq.Parallel.SpoolingTask.SpoolForAll[TInputOutput,TIgnoreKey](QueryTaskGroupState groupState, PartitionedStream`2 partitions, TaskScheduler taskScheduler)
   at System.Linq.Parallel.MergeExecutor`1.Execute()
   at System.Linq.Parallel.MergeExecutor`1.Execute[TKey](PartitionedStream`2 partitions, Boolean ignoreOutput, ParallelMergeOptions options, TaskScheduler taskScheduler, Boolean isOrdered, CancellationState cancellationState, Int32 queryId)
   at System.Linq.Parallel.PartitionedStreamMerger`1.Receive[TKey](PartitionedStream`2 partitionedStream)
   at System.Linq.Parallel.ForAllOperator`1.WrapPartitionedStream[TKey](PartitionedStream`2 inputStream, IPartitionedStreamRecipient`1 recipient, Boolean preferStriping, QuerySettings settings)
   at System.Linq.Parallel.UnaryQueryOperator`2.UnaryQueryOperatorResults.ChildResultsRecipient.Receive[TKey](PartitionedStream`2 inputStream)
   at System.Linq.Parallel.ScanQueryOperator`1.ScanEnumerableQueryOperatorResults.GivePartitionedStream(IPartitionedStreamRecipient`1 recipient)
   at System.Linq.Parallel.UnaryQueryOperator`2.UnaryQueryOperatorResults.GivePartitionedStream(IPartitionedStreamRecipient`1 recipient)
   at System.Linq.Parallel.QueryOperator`1.GetOpenedEnumerator(Nullable`1 mergeOptions, Boolean suppressOrder, Boolean forEffect, QuerySettings querySettings)
   at System.Linq.Parallel.ForAllOperator`1.RunSynchronously()
   at System.Linq.ParallelEnumerable.ForAll[TSource](ParallelQuery`1 source, Action`1 action)
   at Microsoft.VisualStudio.Services.Agent.Util.IOUtil.DeleteDirectory(String path, Boolean contentsOnly, Boolean continueOnContentDeleteError, CancellationToken cancellationToken)
   at Microsoft.VisualStudio.Services.Agent.Util.IOUtil.DeleteDirectory(String path, CancellationToken cancellationToken)
   at Agent.Plugins.Repository.GitSourceProvider.GetSourceAsync(AgentTaskPluginExecutionContext executionContext, RepositoryResource repository, CancellationToken cancellationToken)
   at Agent.Plugins.Repository.CheckoutTask.RunAsync(AgentTaskPluginExecutionContext executionContext, CancellationToken token)
   at Agent.PluginHost.Program.Main(String[] args)
---> (Inner Exception #0) System.UnauthorizedAccessException: Access to the path 'F:\vsagent\9\s\Binaries\Temp\VBCSCompiler\AnalyzerAssemblyLoader\eefc8a0313a344e783a6e8f6c55f7725\9\Microsoft.NetCore.VisualBasic.Analyzers.dll' is denied.
   at System.IO.FileSystem.DeleteFile(String fullPath)
   at System.IO.FileInfo.Delete()
   at Microsoft.VisualStudio.Services.Agent.Util.IOUtil.<>c__DisplayClass9_1.<DeleteDirectory>b__0(FileSystemInfo item)
   at System.Linq.Parallel.ForAllOperator`1.ForAllEnumerator`1.MoveNext(TInput& currentElement, Int32& currentKey)
   at System.Linq.Parallel.ForAllSpoolingTask`2.SpoolingWork()
   at System.Linq.Parallel.SpoolingTaskBase.Work()
   at System.Linq.Parallel.QueryTask.BaseWork(Object unused)
   at System.Linq.Parallel.QueryTask.<>c.<.cctor>b__10_0(Object o)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot)<---

…enced type

…rectives

RikkiGibson · 2018-10-25T20:54:47Z

CI is all good. Please review @AlekseyTs

AlekseyTs

LGTM (iteration 8)

…out-if-error-message * origin/master: (1712 commits) User-defined operator checks should ignore differences in tuple member names (dotnet#30774) Attempted fix for correctly reporting error CS1069 when using implicit namespaces (dotnet#30244) Invert the binding order of InitializerExpressions and CreationExpressions (dotnet#30805) Use Arcade bootstrapping scripts (dotnet#30498) Ensure that the compilers produce double.NaN values in IEEE canonical form. (dotnet#30587) Remove properties set in BeforeCommonTargets.targets Fix publishing of dependent projects Contributing page: reference Unix build instructions Delete 0 Propagate values from AbstractProject to VisualStudioProjectCreationInfo Fix publishing nuget info of dev16.0.x-vs-deps branch Revert "Add a SetProperty API for CPS to passing msbuild properties" Validate generic arguments in `using static` directives (dotnet#30737) Correct 15.9 publish data Enable test. Do not inject attribute types into .Net modules. Add a SetProperty API for CPS to passing msbuild properties Revert "add beta2 suffix to dev16 branch" Fix references Remove commit sha from package versions ...

RikkiGibson requested review from jaredpar, AlekseyTs and a team October 24, 2018 23:01

RikkiGibson commented Oct 24, 2018

View reviewed changes

jaredpar reviewed Oct 24, 2018

View reviewed changes

AlekseyTs reviewed Oct 24, 2018

View reviewed changes

RikkiGibson requested a review from a team October 24, 2018 23:55

cston reviewed Oct 25, 2018

View reviewed changes

cston approved these changes Oct 25, 2018

View reviewed changes

jaredpar reviewed Oct 25, 2018

View reviewed changes

RikkiGibson commented Oct 25, 2018

View reviewed changes

RikkiGibson added 8 commits October 25, 2018 12:42

Add test for using static with generic argument

9a8a383

Check using static constraints during Imports validation

2d73058

Use CorLibrary from _compilation.SourceAssembly instead of from refer…

994475b

…enced type

Use NoLocation.Singleton when @using.UsingDirective is null

f97adcb

Share conversions object between constraint checks on using static di…

94286bf

…rectives

Add expected diagnostic for using static constraint violation

79bd31b

Add nested using static generic constraint test

877fb3a

Add multiple generic constraints in using static test

e723ad8

RikkiGibson force-pushed the using-static-generic-constraint branch from 88e9cca to e723ad8 Compare October 25, 2018 19:42

AlekseyTs approved these changes Oct 25, 2018

View reviewed changes

RikkiGibson merged commit dfa9105 into dotnet:master Oct 25, 2018

RikkiGibson deleted the using-static-generic-constraint branch October 25, 2018 21:13

jcouv mentioned this pull request Apr 17, 2019

error CS0718 in 3.0 SDKs #35062

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate generic arguments in `using static` directives #30737

Validate generic arguments in `using static` directives #30737

RikkiGibson commented Oct 24, 2018

RikkiGibson Oct 24, 2018

RikkiGibson Oct 24, 2018

RikkiGibson Oct 24, 2018 •

edited

Loading

AlekseyTs Oct 24, 2018

jaredpar Oct 24, 2018

AlekseyTs Oct 24, 2018 •

edited

Loading

RikkiGibson Oct 24, 2018

jaredpar Oct 24, 2018

AlekseyTs Oct 24, 2018 •

edited

Loading

RikkiGibson Oct 24, 2018

cston Oct 25, 2018

RikkiGibson Oct 25, 2018

jaredpar Oct 25, 2018

RikkiGibson Oct 25, 2018

RikkiGibson Oct 25, 2018

AlekseyTs Oct 25, 2018

RikkiGibson Oct 25, 2018

RikkiGibson commented Oct 25, 2018

RikkiGibson commented Oct 25, 2018

AlekseyTs left a comment

Validate generic arguments in using static directives #30737

Validate generic arguments in using static directives #30737

Conversation

RikkiGibson commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Oct 25, 2018

RikkiGibson commented Oct 25, 2018

AlekseyTs left a comment

Choose a reason for hiding this comment

Validate generic arguments in `using static` directives #30737

Validate generic arguments in `using static` directives #30737

RikkiGibson Oct 24, 2018 •

edited

Loading

AlekseyTs Oct 24, 2018 •

edited

Loading

AlekseyTs Oct 24, 2018 •

edited

Loading