Skip to content

Commit

Permalink
Fixup nullable annotation
Browse files Browse the repository at this point in the history
Had to clean up a few nullable annotations now that we are compiling
agaist `netcoreapp3.1` and hence get the full value of the framework
annotations.

This is also problematic though because there are now two places where
the compiler can see nullable attributes that are directly used by the
developer. For example `NotNullWhenAttribute`. This is both defined in
our assemblies for non-netcoreapp target frameworks and provided by the
SDK when targeting `netcoreapp3.1`.

This causes a problem for assemblies which have the following
characteristics:

1. Target `netcoreapp3.1`
1. Reference an assembly targeting `netstandard2.0` which uses our
nullable attributes definition
1. Has IVT into (2) above

These properties essentially define all of our unit test assemblies. In
that environment it's not possible to use nullable attributes in code
because the compiler can't disambiguate which definition of
`NotNullWhenAttribute` to use. This meant I had to temporarily remove a
few attributes until we can complete dotnet#40766.
  • Loading branch information
jaredpar committed Jan 30, 2020
1 parent e115509 commit 5a0b78c
Show file tree
Hide file tree
Showing 10 changed files with 42 additions and 30 deletions.
8 changes: 4 additions & 4 deletions src/Compilers/Core/CommandLine/CompilerServerLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ static CompilerServerLogger()
try
{
// Check if the environment
string loggingFileName = Environment.GetEnvironmentVariable(environmentVariable);
string? loggingFileName = Environment.GetEnvironmentVariable(environmentVariable);

if (loggingFileName != null)
{
Expand Down Expand Up @@ -75,15 +75,15 @@ public static void Initialize(string outputPrefix)
/// <summary>
/// Log an exception. Also logs information about inner exceptions.
/// </summary>
public static void LogException(Exception e, string reason)
public static void LogException(Exception exception, string reason)
{
if (s_loggingStream != null)
{
Log("Exception '{0}' occurred during '{1}'. Stack trace:\r\n{2}", e.Message, reason, e.StackTrace);
Log("Exception '{0}' occurred during '{1}'. Stack trace:\r\n{2}", exception.Message, reason, exception.StackTrace);

int innerExceptionLevel = 0;

e = e.InnerException;
Exception? e = exception.InnerException;
while (e != null)
{
Log("Inner exception[{0}] '{1}'. Stack trace: \r\n{1}", innerExceptionLevel, e.Message, e.StackTrace);
Expand Down
19 changes: 14 additions & 5 deletions src/Compilers/Core/MSBuildTask/ManagedCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,12 @@ public string? SubsystemVersion

public string? TargetType
{
set { _store[nameof(TargetType)] = CultureInfo.InvariantCulture.TextInfo.ToLower(value); }
set
{
_store[nameof(TargetType)] = value != null
? CultureInfo.InvariantCulture.TextInfo.ToLower(value)
: null;
}
get { return (string?)_store[nameof(TargetType)]; }
}

Expand Down Expand Up @@ -467,11 +472,11 @@ protected override int ExecuteTool(string pathToTool, string responseFileCommand
try
{
var workingDir = CurrentDirectoryToUse();
string tempDir = BuildServerConnection.GetTempPath(workingDir);
string? tempDir = BuildServerConnection.GetTempPath(workingDir);

if (!UseSharedCompilation ||
HasToolBeenOverridden ||
!BuildServerConnection.IsCompilerServerSupported(tempDir))
!BuildServerConnection.IsCompilerServerSupported)
{
return base.ExecuteTool(pathToTool, responseFileCommands, commandLineCommands);
}
Expand All @@ -483,6 +488,10 @@ protected override int ExecuteTool(string pathToTool, string responseFileCommand
CompilerServerLogger.Log($"BuildResponseFile = '{responseFileCommands}'");

var clientDir = Path.GetDirectoryName(PathToManagedTool);
if (clientDir is null || tempDir is null)
{
return base.ExecuteTool(pathToTool, responseFileCommands, commandLineCommands);
}

// Note: we can't change the "tool path" printed to the console when we run
// the Csc/Vbc task since MSBuild logs it for us before we get here. Instead,
Expand Down Expand Up @@ -567,10 +576,10 @@ private string CurrentDirectoryToUse()
/// <summary>
/// Get the "LIB" environment variable, or NULL if none.
/// </summary>
private string LibDirectoryToUse()
private string? LibDirectoryToUse()
{
// First check the real environment.
string libDirectory = Environment.GetEnvironmentVariable("LIB");
string? libDirectory = Environment.GetEnvironmentVariable("LIB");

// Now go through additional environment variables.
string[] additionalVariables = EnvironmentVariables;
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/MSBuildTask/MapSourceRoots.cs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ private static bool EndsWithDirectorySeparator(string path)

private void MergeSourceRootMetadata(ITaskItem left, ITaskItem right)
{
foreach (string metadataName in right.MetadataNames)
foreach (string? metadataName in right.MetadataNames)
{
var leftValue = left.GetMetadata(metadataName);
var rightValue = right.GetMetadata(metadataName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
<AssemblyVersion/>
<!-- CA1819 (Properties should not return arrays) disabled as it is very common across this project. -->
<NoWarn>$(NoWarn);CA1819</NoWarn>

<!-- NuGet -->
<IsPackable>true</IsPackable>
<PackageId>Microsoft.CodeAnalysis.Build.Tasks</PackageId>
Expand All @@ -23,6 +23,12 @@
</PackageDescription>
<GenerateMicrosoftCodeAnalysisCommitHashAttribute>true</GenerateMicrosoftCodeAnalysisCommitHashAttribute>
</PropertyGroup>

<!-- Disable the nullable warnings when not compiling for netcoreapp3.1 -->
<PropertyGroup Condition="'$(TargetFramework)' != 'netcoreapp3.1'">
<NoWarn>$(NoWarn);8600;8601;8602;8603;8604</NoWarn>
</PropertyGroup>

<ItemGroup>
<Content Include="*.targets">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/MSBuildTask/MvidReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ private static Guid FindMvidInSections(ushort count, BinaryReader reader)
return s_empty;
}

if (name.Length == 8 && name[0] == '.' &&
if (name!.Length == 8 && name[0] == '.' &&
name[1] == 'm' && name[2] == 'v' && name[3] == 'i' && name[4] == 'd' && name[5] == '\0')
{
// Section: VirtualSize (4)
Expand Down Expand Up @@ -150,7 +150,7 @@ private static Guid ReadMvidSection(BinaryReader reader, uint pointerToMvidSecti
return s_empty;
}

return new Guid(guidBytes);
return new Guid(guidBytes!);
}

private static bool ReadUInt16(BinaryReader reader, out ushort output)
Expand All @@ -177,7 +177,7 @@ private static bool ReadUInt32(BinaryReader reader, out uint output)
return true;
}

private static bool ReadBytes(BinaryReader reader, int count, [NotNullWhen(true)] out byte[]? output)
private static bool ReadBytes(BinaryReader reader, int count, out byte[]? output)
{
if (reader.BaseStream.Position + count >= reader.BaseStream.Length)
{
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/Core/MSBuildTask/Utilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,8 @@ internal static string GenerateFullPathToTool(string toolName)
var assemblyDirectory = Path.GetDirectoryName(assemblyPath);

return RuntimeHostInfo.IsDesktopRuntime
? Path.Combine(assemblyDirectory, toolName)
: Path.Combine(assemblyDirectory, "bincore", toolName);
? Path.Combine(assemblyDirectory!, toolName)
: Path.Combine(assemblyDirectory!, "bincore", toolName);
}
}
}
4 changes: 2 additions & 2 deletions src/Compilers/Core/MSBuildTask/ValidateBootstrap.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public sealed partial class ValidateBootstrap : Task
public string? TasksAssemblyFullPath
{
get { return _tasksAssemblyFullPath; }
set { _tasksAssemblyFullPath = NormalizePath(Path.GetFullPath(value)); }
set { _tasksAssemblyFullPath = NormalizePath(Path.GetFullPath(value!)); }
}

public ValidateBootstrap()
Expand Down Expand Up @@ -103,7 +103,7 @@ public override bool Execute()
return path;
}

private string GetDirectory(Assembly assembly) => Path.GetDirectoryName(Utilities.TryGetAssemblyPath(assembly));
private string? GetDirectory(Assembly assembly) => Path.GetDirectoryName(Utilities.TryGetAssemblyPath(assembly));

internal static void AddFailedLoad(AssemblyName name)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// This was copied from https://github.com/dotnet/coreclr/blob/60f1e6265bd1039f023a82e0643b524d6aaf7845/src/System.Private.CoreLib/shared/System/Diagnostics/CodeAnalysis/NullableAttributes.cs
// and updated to have the scope of the attributes be internal.

#if NETSTANDARD2_0 || NET472 || NET20 || NETSTANDARD1_3 || NET45
#if !NETCOREAPP
#nullable enable

namespace System.Diagnostics.CodeAnalysis
Expand Down Expand Up @@ -88,7 +88,4 @@ internal sealed class DoesNotReturnIfAttribute : Attribute
}
}

#elif NETCOREAPP3_1
#else
#error "Unsupported configuration"
#endif
12 changes: 6 additions & 6 deletions src/Compilers/Shared/BuildServerConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ namespace Microsoft.CodeAnalysis.CommandLine
/// communication layer which is shared between MSBuild and non-MSBuild components. This is the problem that
/// BuildPathsAlt fixes as the type lives with the build server communication code.
/// </summary>
internal readonly struct BuildPathsAlt
internal sealed class BuildPathsAlt
{
/// <summary>
/// The path which contains the compiler binaries and response files.
Expand Down Expand Up @@ -77,15 +77,15 @@ internal sealed class BuildServerConnection
/// <summary>
/// Determines if the compiler server is supported in this environment.
/// </summary>
internal static bool IsCompilerServerSupported(string tempPath) => GetPipeNameForPathOpt("") is object;
internal static bool IsCompilerServerSupported => GetPipeNameForPathOpt("") is object;

public static Task<BuildResponse> RunServerCompilation(
RequestLanguage language,
string? sharedCompilationId,
List<string> arguments,
BuildPathsAlt buildPaths,
string? keepAlive,
string libEnvVariable,
string? libEnvVariable,
CancellationToken cancellationToken)
{
var pipeNameOpt = sharedCompilationId ?? GetPipeNameForPathOpt(buildPaths.ClientDirectory);
Expand All @@ -108,7 +108,7 @@ internal static async Task<BuildResponse> RunServerCompilationCore(
BuildPathsAlt buildPaths,
string? pipeName,
string? keepAlive,
string libEnvVariable,
string? libEnvVariable,
int? timeoutOverride,
CreateServerFunc createServerFunc,
CancellationToken cancellationToken)
Expand Down Expand Up @@ -588,7 +588,7 @@ internal static string GetClientMutexName(string pipeName)
/// is <paramref name="workingDir"/>. This function must emulate <see cref="Path.GetTempPath"/> as
/// closely as possible.
/// </summary>
public static string GetTempPath(string? workingDir)
public static string? GetTempPath(string? workingDir)
{
if (PlatformInformation.IsUnix)
{
Expand Down Expand Up @@ -656,7 +656,7 @@ internal sealed class FileMutex : IDisposable
internal static string GetMutexDirectory()
{
var tempPath = BuildServerConnection.GetTempPath(null);
var result = Path.Combine(tempPath, ".roslyn");
var result = Path.Combine(tempPath!, ".roslyn");
Directory.CreateDirectory(result);
return result;
}
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/Shared/RuntimeHostInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ internal static (string processFilePath, string commandLineArguments, string too
if (IsDotNetHost(out string? pathToDotNet))
{
commandLineArguments = $@"exec ""{toolFilePath}"" {commandLineArguments}";
return (pathToDotNet, commandLineArguments, toolFilePath);
return (pathToDotNet!, commandLineArguments, toolFilePath);
}
else
{
Expand All @@ -58,7 +58,7 @@ internal static NamedPipeClientStream CreateNamedPipeClient(string serverName, s

private static string DotNetHostPathEnvironmentName = "DOTNET_HOST_PATH";

private static bool IsDotNetHost([NotNullWhen(true)] out string? pathToDotNet)
private static bool IsDotNetHost(out string? pathToDotNet)
{
pathToDotNet = GetDotNetPathOrDefault();
return true;
Expand Down

0 comments on commit 5a0b78c

Please sign in to comment.