-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Disable implicit imports in tools #55828
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.
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
Conversation
We introduced these light weight Linq overloads to fix startup time of crossgen2. Is crossgen2 going to use the heavy weight Linq with this change? What is it going to do to the startup time? |
<IsShipping>false</IsShipping> | ||
<SignAssembly>false</SignAssembly> | ||
<RunAnalyzers>false</RunAnalyzers> | ||
<DisableImplicitNamespaceImports_DotNet>true</DisableImplicitNamespaceImports_DotNet> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering whether we should have it disabled for the whole dotnet/runtime repo. It is very easy to fall into Linq performance trap by accident... cc @stephentoub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SDK will be bringing in these:
https://github.com/dotnet/sdk/blob/55195f3c4ba3f582aa91ec824bd24a7ba3e73528/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.DefaultItems.props#L91-L97
I actually expected these would cause build failures once we started using a new enough SDK in runtime, since many of our assemblies don't or can't (because of layering) reference assemblies that define these namespaces, and my understanding is it effectively acts as if you directly added these using to each file, which would normally fail to compile in such cases. Either way, at a minimum we should reduce the list we automatically import (e.g. removing LINQ as you suggest), but I'd also be fine starting by disabling this entirely for the repo. If we do the latter, I expect in time we'll want to relax that, e.g. it'd be nice to eliminate lots of usings from the tops of most test files. There are other changes coming, e.g. file-scoped namespaces, that will likely cause us to overhaul lots of files in the repo as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out we already disable it. We set:
runtime/src/libraries/Directory.Build.targets
Lines 54 to 56 in 405da67
<DisableImplicitFrameworkReferences Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and | |
$([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '$(NETCoreAppCurrentVersion)')) and | |
('$(IsNETCoreAppRef)' == 'true' or '$(IsNETCoreAppSrc)' == 'true')">true</DisableImplicitFrameworkReferences> |
which in turn sets DisableImplicitNamespaceImports:
https://github.com/dotnet/sdk/blob/714082d0e96cef8c3a5f5d04a08d18baaf036f21/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L42
which in turn sets DisableImplicitNamespaceImports_DotNet:
https://github.com/dotnet/sdk/blob/85943958e5a01d6bbd39a947aec2a4a37b8ba7cf/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateImplicitNamespaceImports.targets#L22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that affects just libraries, correct? And this PR still correct, since it is for tools.
Also curious, does rationale from #55855 (comment) applied to libs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that affects just libraries, correct? And this PR still correct, since it is for tools.
Yes, that's just libs, and this PR is still valid for tools.
Also curious, does rationale from #55855 (comment) applied to libs?
We're currently not interested in importing namespaces in that manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Discovered in NativeAOT https://github.com/dotnet/runtimelab/issues/1334
Closes #55855