Annotate CommandLineParser/Arguments and File/PathUtilities by jcouv · Pull Request #41063 · dotnet/roslyn

jcouv · 2020-01-17T23:29:03Z

This found a bug (added tests and a simple fix in CommandLineParser.EnumerateFiles).
I also added a null check in CommandLineParser (line 428) which I could not protect or rationalize otherwise.

sharwell · 2020-01-18T00:49:08Z

src/Compilers/Core/Portable/CommandLine/CommandLineArguments.cs

        /// Gets the emit options.
        /// </summary>
-        public EmitOptions EmitOptions { get; internal set; }
+        public EmitOptions EmitOptions { get; internal set; } = null!; // initialized by Parse


❓ Why not make this a constructor parameter (required initialization)? #Resolved

Tried it after your suggestion, but decided against it as it breaks the regularity of the code in parser and also implies a change in VB code. Neither of these seem worth it.
The topic of field initializers will be re-discussed in LDM and at the very least we'll provide some guidance to the recommended pattern for dealing with this situation.

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

src/Compilers/Core/Portable/FileSystem/PathUtilities.cs

sharwell · 2020-01-18T00:55:13Z

src/Compilers/Core/Portable/RuleSet/RuleSet.cs


-        internal static ReportDiagnostic GetDiagnosticOptionsFromRulesetFile(string rulesetFileFullPath, out Dictionary<string, ReportDiagnostic> diagnosticOptions, IList<Diagnostic> diagnosticsOpt, CommonMessageProvider messageProviderOpt)
+#nullable enable
+        internal static ReportDiagnostic GetDiagnosticOptionsFromRulesetFile(string? rulesetFileFullPath, out Dictionary<string, ReportDiagnostic> diagnosticOptions, IList<Diagnostic> diagnosticsOpt, CommonMessageProvider messageProviderOpt)


Suggested change

internal static ReportDiagnostic GetDiagnosticOptionsFromRulesetFile(string? rulesetFileFullPath, out Dictionary<string, ReportDiagnostic> diagnosticOptions, IList<Diagnostic> diagnosticsOpt, CommonMessageProvider messageProviderOpt)

internal static ReportDiagnostic GetDiagnosticOptionsFromRulesetFile(string? rulesetFileFullPath, out Dictionary<string, ReportDiagnostic> diagnosticOptions, IList<Diagnostic>? diagnosticsOpt, CommonMessageProvider? messageProviderOpt)

``` #Resolved

jcouv · 2020-01-24T03:36:28Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

            }

-            if (string.IsNullOrEmpty(filePath))
+            if (RoslynString.IsNullOrWhiteSpace(filePath))


📝 this change aligns with logic in ParseResourceDescription (called a few lines above). The effect is to change which error is reported (see ParseResources test, which was updated). #Resolved

jcouv · 2020-01-24T03:38:04Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

-            if (!string.IsNullOrEmpty(outputDirectory) && baseDirectory != outputDirectory)
+            if (RoslynString.IsNullOrEmpty(outputDirectory))
+            {
+                AddDiagnostic(diagnostics, ErrorCode.ERR_NoOutputDirectory);


📝 this new error guards against null output directory, allowing us to declare OutputDirectory as non-nullable. #Resolved

📝 This new error means loc change, and the cutoff for 16.5 already passed so this needs to retarget 16.6 Preview 1 #Resolved

What was the previous behavior?

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

You could get a null OutputDirectory, which we're not prepared to handle in code that reads that property.
I'm not sure if that emitting code (which reads the property) could be reached though.

In reply to: 377830798 [](ancestors = 377830798,370462374)

jcouv · 2020-01-29T20:05:36Z

@dotnet/roslyn-compiler @sharwell for review. This annotates more of the public APIs. Thanks #Resolved

jcouv · 2020-01-31T15:19:05Z

@dotnet/roslyn-compiler @sharwell for review. This annotates more of the public APIs. Thanks #Resolved

sharwell

Marking request changes just for the retarget due to loc change

sharwell · 2020-01-31T16:52:04Z

.../Def/Implementation/ProjectSystem/MetadataReferences/VisualStudioMetadataReferenceManager.cs

                }

-                var moduleFileKey = FileKey.Create(PathUtilities.CombineAbsoluteAndRelativePaths(assemblyDir, moduleName));
+                var moduleFileKey = FileKey.Create(PathUtilities.CombineAbsoluteAndRelativePaths(assemblyDir!, moduleName));


💡 Replacing the condition above would remove the need for this suppression:

-if (moduleBuilder.Count == 0) +if (assemblyDir is null)

If that's too confusing, an equally usable alternative is moving the initialization of assemblyDir outside that if statement:

assemblyDir ??= Path.GetDirectoryName(fileKey.FullPath); ``` #Resolved

sharwell · 2020-01-31T16:54:57Z

src/Compilers/Core/Portable/CommandLine/CommandLineArguments.cs

+            if (references.IsDefaultOrEmpty && messageProviderOpt != null)
            {
-                diagnosticsOpt.Add(new DiagnosticInfo(messageProviderOpt, messageProviderOpt.ERR_MetadataFileNotFound, cmdReference.Reference));
+                diagnosticsOpt!.Add(new DiagnosticInfo(messageProviderOpt, messageProviderOpt.ERR_MetadataFileNotFound, cmdReference.Reference));


💭 If you're going to need a suppression either way, why not keep the original check and place the suppression on messageProviderOpt?

Edit: Oh, because two suppressions. In that case, I vote for the original code but add:

RoslynDebug.AssertNotNull(messageProviderOpt); ``` #Resolved

sharwell · 2020-01-31T17:00:36Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

-            if (!string.IsNullOrEmpty(outputDirectory) && baseDirectory != outputDirectory)
+            if (RoslynString.IsNullOrEmpty(outputDirectory))
+            {
+                AddDiagnostic(diagnostics, ErrorCode.ERR_NoOutputDirectory);


📝 This new error means loc change, and the cutoff for 16.5 already passed so this needs to retarget 16.6 Preview 1 #Resolved

sharwell · 2020-01-31T17:01:24Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

+        private static IEnumerable<string> ParseUsings(string arg, string? value, IList<Diagnostic> diagnostics)
        {
-            if (value.Length == 0)
+            if (value is null || value.Length == 0)


Suggested change

if (value is null || value.Length == 0)

if (RoslynString.IsNullOrEmpty(value))

``` #Resolved

jaredpar · 2020-02-12T07:20:40Z

src/Compilers/Core/Portable/CommandLine/CommandLineArguments.cs

        /// Compilation name or null if not specified.
        /// </summary>
-        public string CompilationName { get; internal set; }
+        public string? CompilationName { get; internal set; }


I distinctly remember fixing a bug that came about specifically because the code didn't realize CompilationName could be null. Seeing this makes me happy for the time I spent fixing that bug some years ago. #Resolved

jaredpar · 2020-02-12T07:25:08Z

src/Compilers/Core/Portable/CommandLine/CommandLineParser.cs

        /// Trims all '.' and whitespace from the end of the path
        /// </summary>
-        internal static string RemoveTrailingSpacesAndDots(string path)
+        internal static string? RemoveTrailingSpacesAndDots(string? path)


Did you consider using NotNullWhenNotNull to mark the return? #Resolved

jaredpar · 2020-02-12T07:27:35Z

src/Compilers/Core/Portable/CommandLine/CommandLineParser.cs

                        }

-                        if (responsePaths != null)
+                        if (responsePaths != null &&


Feels like we're hiding a bug here. Agree this change is necessary because the old code path would essentially throw in the case where GetDirectoryName returned null. At the same time though now this will just silently continue. Should we issue the diagnostic FTL_InvalidInputFileName when we can't get a directory name here? #Closed

Makes sense.
I'll add the diagnostic. I've not been able to hit this case yet though, so it will lack coverage...

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

jaredpar · 2020-02-12T07:30:26Z

src/Compilers/Core/Portable/FileSystem/FileUtilities.cs

                    }

-                    if (path.Length == 1)
+                    if (path!.Length == 1)


Seems like path could be null in the case here where kind == PathKind.Empty #Resolved

See assertion I added above. In this branch, we know that GetPathKind(path) == kind == PathKind.RelativeToCurrentDirectory

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

jaredpar · 2020-02-12T07:31:54Z

Done with review pass (iteration 12) #Resolved

cston · 2020-02-13T00:15:14Z

src/Scripting/CSharpTest/CommandLineRunnerTests.cs

+        [Theory]
+        [InlineData(null, null)]
+        [InlineData("c:", null)]
+        [InlineData("c:\\", null)]


"c:\" [](start = 20, length = 6)

Are these tests only run on Windows? #Closed

I thought the logic in the method didn't depend on the current platform, but it looks like it does (directory separator is \ or /). I'll make the test conditional.

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

cston · 2020-02-13T00:17:09Z

src/Scripting/CSharpTest/CommandLineRunnerTests.cs

+        [InlineData("\\first\\second", "\\first")]
+        [InlineData("\\first\\second\\", "\\first\\second")]
+        [InlineData("\\first\\second\\third", "\\first\\second")]
+        public void TestGetDirectoryName_Windows(string path, string expectedOutput)


public [](start = 8, length = 6)

Do we need to test relative paths as well? #Closed

Doesn't hurt. I'll add.
The main point of this test is to show when we're returning null

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

master is now targeting 16.6, so no loc constraint

333fred · 2020-02-14T18:25:29Z

                    string referencePath = Path.Combine(directory!, referenceName + ".dll");

Consider using an assert instead of a suppression. #Resolved

Refers to: src/Compilers/Core/Portable/AssemblyUtilities.cs:63 in 886d453. [](commit_id = 886d453, deletion_comment = False)

333fred · 2020-02-14T18:26:20Z

src/Compilers/Core/Portable/CommandLine/CommandLineParser.cs

            else
            {
-                pdbPath = Path.ChangeExtension(Path.Combine(outputDirectory, outputFileName), ".pdb");
+                // If outputDirectory were null, then outputFileName would be null (see ParseAndNormalizeFile)


These comments are better served with an assert for verification. #Resolved

333fred

LGTM (commit 20) with assertion changes.

jcouv · 2020-02-14T20:01:30Z

                    string referencePath = Path.Combine(directory!, referenceName + ".dll");

Hum, this is an existing suppression, and I'm not sure it's justified... I'll add to tracking issue for null directories

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

Refers to: src/Compilers/Core/Portable/AssemblyUtilities.cs:63 in 886d453. [](commit_id = 886d453, deletion_comment = False)

jaredpar · 2020-02-14T22:02:15Z

@jcouv @333fred

Hum, this is an existing suppression, and I'm not sure it's justified...

It's justified. The rational is captured here #41485 (comment)

jaredpar · 2020-02-14T22:28:10Z

@jcouv @333fred

At the same time though several people have asked about why this suppression exists. Clearly the context is unclear. At the least I should add a comment here to justify why this is done this way.

jcouv · 2020-02-15T00:28:40Z

@jaredpar I’ll update the comment

jcouv · 2020-02-15T05:18:00Z

@jaredpar I'm going to leave the comment as-is for follow-up. Reading the rationale again, I suspect that it is incorrect. If you open a file in the root directory, the OpenRead would succeed, but GetDirectoryName would return null (that's really annoying behavior in top-level directory in my opinion, but that seems to be the case).

I'll go ahead and merge leaving the follow-up comment.

jcouv added Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types labels Jan 17, 2020

jcouv self-assigned this Jan 17, 2020

Annotate CommandLineParser/Arguments and File/PathUtilities

9c3fbf3

jcouv force-pushed the annotate-cmd branch from b9bf4fa to 9c3fbf3 Compare January 17, 2020 23:54

sharwell reviewed Jan 18, 2020

View reviewed changes

src/Compilers/Core/Portable/FileSystem/PathUtilities.cs Outdated Show resolved Hide resolved

sharwell reviewed Jan 18, 2020

View reviewed changes

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 18, 2020

jcouv force-pushed the annotate-cmd branch from 0984ea1 to d2f5775 Compare January 21, 2020 06:04

WIP

5c4fc65

jcouv force-pushed the annotate-cmd branch from d2f5775 to 5c4fc65 Compare January 21, 2020 06:07

Julien Couvreur and others added 2 commits January 21, 2020 14:49

More bootstrapping

ec47ea0

Update test baseline

a9b41a4

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 24, 2020

jcouv commented Jan 24, 2020

View reviewed changes

jcouv marked this pull request as ready for review January 24, 2020 05:02

jcouv requested review from a team as code owners January 24, 2020 05:02

jcouv and others added 6 commits January 23, 2020 21:04

Merge branch 'master' into annotate-cmd

a5a9e84

Update ErrorCode.cs

1976d2b

Fix bad implementation of NotNullWhen

617efb4

Fix a bug

bacc031

Make two new tests conditional

f698923

Make one more test conditional

ffc4128

sharwell previously requested changes Jan 31, 2020

View reviewed changes

Address PR feedback

895f69d

jaredpar reviewed Feb 12, 2020

View reviewed changes

Julien Couvreur added 3 commits February 12, 2020 13:33

Merge remote-tracking branch 'dotnet/master' into annotate-cmd

4a5932e

Address feedback from Jared

fdf7ee5

Fixups following merge

d238dab

jcouv mentioned this pull request Feb 13, 2020

Some callers of PathUtilities.GetDirectoryName don't handle null returned values #41636

Open

Tag some remaining issues

d9da4c4

jcouv requested a review from a team as a code owner February 13, 2020 00:04

cston reviewed Feb 13, 2020

View reviewed changes

Address feedback from Chuck

c72247f

jaredpar approved these changes Feb 13, 2020

View reviewed changes

Julien Couvreur added 3 commits February 13, 2020 09:38

Fixing regression

0806f66

Fix merge conflict in ErrorCode

3f49be0

Merge remote-tracking branch 'dotnet/master' into annotate-cmd

886d453

333fred reviewed Feb 14, 2020

View reviewed changes

333fred approved these changes Feb 14, 2020

View reviewed changes

Address feedback from Fred

2430382

jcouv merged commit 092d850 into dotnet:master Feb 15, 2020

jcouv deleted the annotate-cmd branch February 15, 2020 05:18

	internal static ReportDiagnostic GetDiagnosticOptionsFromRulesetFile(string? rulesetFileFullPath, out Dictionary<string, ReportDiagnostic> diagnosticOptions, IList<Diagnostic> diagnosticsOpt, CommonMessageProvider messageProviderOpt)
	internal static ReportDiagnostic GetDiagnosticOptionsFromRulesetFile(string? rulesetFileFullPath, out Dictionary<string, ReportDiagnostic> diagnosticOptions, IList<Diagnostic>? diagnosticsOpt, CommonMessageProvider? messageProviderOpt)
	``` #Resolved

	if (value is null \|\| value.Length == 0)
	if (RoslynString.IsNullOrEmpty(value))
	``` #Resolved

Conversation

jcouv commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell Jan 18, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sharwell Jan 18, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Jan 31, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

sharwell Jan 31, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Jan 31, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Jan 31, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Jan 31, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Feb 12, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Feb 12, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

jaredpar Feb 12, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2020

Choose a reason for hiding this comment

jcouv commented Jan 17, 2020 •

edited

Loading

sharwell Jan 18, 2020 •

edited by jcouv

Loading

sharwell Jan 18, 2020 •

edited by jcouv

Loading

jcouv Jan 24, 2020 •

edited

Loading

jcouv Jan 24, 2020 •

edited

Loading

sharwell Jan 31, 2020 •

edited by jcouv

Loading

jcouv commented Jan 29, 2020 •

edited

Loading

jcouv commented Jan 31, 2020 •

edited

Loading

sharwell Jan 31, 2020 •

edited by jcouv

Loading

sharwell Jan 31, 2020 •

edited by jcouv

Loading

sharwell Jan 31, 2020 •

edited by jcouv

Loading

sharwell Jan 31, 2020 •

edited by jcouv

Loading

jaredpar Feb 12, 2020 •

edited

Loading

jaredpar Feb 12, 2020 •

edited by jcouv

Loading

jaredpar Feb 12, 2020 •

edited by 333fred

Loading

jaredpar Feb 12, 2020 •

edited by jcouv

Loading

jaredpar commented Feb 12, 2020 •

edited by jcouv

Loading

cston Feb 13, 2020 •

edited by 333fred

Loading

cston Feb 13, 2020 •

edited by 333fred

Loading

333fred commented Feb 14, 2020 •

edited by jcouv

Loading

333fred Feb 14, 2020 •

edited by jcouv

Loading