Take advantage of C# 11 #6164
Take advantage of C# 11 #6164
Conversation
This change is 100% mechanical, applying the code fix for the solution, and only contains files that didn't end up with warnings after the fix as applied.
The vsHostWorkspaceServicesProvider parameter was being checked for null despite being nullable, and an optional MEF import.
This was previously validating a nullable parameter, which doesn't make sense, so made the parameter non-nullable, and changed call sites.
This comment was marked as outdated.
This comment was marked as outdated.
This was null checking a nullable parameter, which doesn't make sense, so made the parameter non-nullable. Callers just suppress so runtime behaviour is unchanged
This was null checking a nullable parameter, which doesn't make sense, so made the parameter non-nullable. Callers just suppress so runtime behaviour is unchanged
davidwengier
force-pushed
the
NewCSharpHotness
branch
from
f3dde0c to
98128bb
Compare
davidwengier
requested review from
a team,
NTaylorMullen,
ryanbrandenburg,
allisonchou and
sharwell
as code owners
!!
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/ProjectExtensions.cs
Show resolved
Hide resolved
| if (vsHostWorkspaceServicesProvider is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(vsHostWorkspaceServicesProvider)); | ||
| } |
There was a problem hiding this comment.
😕 Why are we removing this line instead of making the argument required?
There was a problem hiding this comment.
📝 Note that null checking an optional MEF import is intentional in some cases:
dotnet/roslyn#59047
There was a problem hiding this comment.
I'm confused.. when you say "make the argument required" do you mean remove the AllowDefault? If so, well because I assume the MEF import sometime isn't satisfied, and if not, well that just seems counter intuitive.
There was a problem hiding this comment.
Gonna merge this, as with so many changes its a merge conflict waiting to happen. Will follow up if there is any action here.
There was a problem hiding this comment.
The desired outcome here is to continue to have [Import(AllowDefault = true)], but also have !! on the parameter. This results in a MEF 2 composition which will not fail to compose for lack of this part, but will throw an exception if anything tries to obtain this part. For example, if the catalog doesn't provide vsHostWorkspaceServicesProvider, it would be perfectly acceptable to obtain Lazy<RazorLanguageServerClient> from the catalog, but you could not access Lazy<RazorLanguageServerClient>.Value without an exception.
There was a problem hiding this comment.
➡️ Filed follow-up issue #6165
There was a problem hiding this comment.
Interesting.. and weird :P
Does it change your thinking at all knowing that the code that uses the service later was already defensive against nulls? I took that as a sign that there are legitimate scenarios where the part might not be available (which granted, either isn't true any more, or wasn't working anyway because it would have thrown an exception.. but I imagine there is a non-zero amount of scenarios that aren't working because we throw exceptions)
...Microsoft.VisualStudio.LanguageServerClient.Razor/Logging/LogHubLoggerProviderFactoryBase.cs
Show resolved
Hide resolved
...r/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/TagHelperServiceTestBase.cs
Outdated
Show resolved
Hide resolved
Fixes: My own personal issues.
Commit-by-commit review thoroughly recommended, so you can skip the mechanical change and look at the interesting ones separately. The second commit, with the message that is just "Simplify null checks" is 100% mechanical, and will presumably kill github 😬