Skip to content
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

Add and fix nullability annotations in Microsoft.CodeAnalysis.Razor.Workspaces #8115

Merged
merged 7 commits into from Feb 1, 2023

Conversation

DustinCampbell
Copy link
Member

@DustinCampbell DustinCampbell commented Jan 11, 2023

Fixes #7945

This starts the work of updating Microsoft.CodeAnalysis.Razor.Workspaces with nullable annotations, making some largish refactorings, and generally cleaning up.
Here are a few highlights:

  • Added AssumesNotNull() extension methods that can be used to verify that an expression is non-null. This is similar to the Assume.NotNull(...) methods from the Microsoft.VisualStudio.Validation library. The advantage to the extension method forms is that they can be used at an expression level, which makes them simpler to use than Assume.NotNull(...), which often requires requiring expressions into statements in order to validate.
  • Added or corrected nullable annotations to much of the "project system-y" types in Workspaces.
  • Converted DocumentSnapshot to an interface: IDocumentSnapshot. The old DefaultDocumentSnapshot has been renamed to DocumentSnapshot.
  • Converted ProjectSnapshot to an interface: IProjectSnapshot. The old DefaultProjectSnapshot has been renamed to ProjectSnapshot.
  • Converted ErrorReporter to an interface: IErrorReporter. The old DefaultErrorReporter has been renamed to ErrorReporter. In addition, I've updated the test infrastructure and all tests to use a test-only IErrorReporter that logs exceptions to the test output.

There is more to come, but this PR is already large enough, so I'm submitting it for review.

Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MOAR PATTERNS!

@DustinCampbell DustinCampbell force-pushed the nullability branch 5 times, most recently from 63688e8 to a25548d Compare January 30, 2023 23:01
private class DefaultElementCompletionResult : ElementCompletionResult
{
private readonly IReadOnlyDictionary<string, IEnumerable<TagHelperDescriptor>> _completions;
var readonlyCompletions = new Dictionary<string, IEnumerable<TagHelperDescriptor>>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the second method I've seen that converts a dictionary from something to something very very similar. I have nothing but questions, but I don't expect you to answer them. In fact, I admire your restraint in leaving them here :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I tried to remove them but then I saw that this is public and used by Web Tools. So, I left it mostly in place and just removed the gratuitous inheritance.

@@ -384,12 +393,16 @@ private void UpdateProjectDocuments(IReadOnlyList<DocumentSnapshotHandle> docume
}
}

private void MoveDocument(string documentFilePath, DefaultProjectSnapshot fromProject, DefaultProjectSnapshot toProject)
private void MoveDocument(string documentFilePath, ProjectSnapshot fromProject, ProjectSnapshot toProject)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious if this makes more sense as IProjectSnapshot?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might, but understanding the impact of that is a bit beyond the scope of this PR. This change is just the result of a rename.

Task<RazorCodeDocument> GetGeneratedOutputAsync();

bool TryGetText([NotNullWhen(true)] out SourceText? result);
bool TryGetTextVersion(out VersionStamp result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we fail to get the VersionStamp?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's default. This is a struct, not a reference type, and it's not necessary to lift structs to nullable value types in TryGet methods.

@DustinCampbell
Copy link
Member Author

Thanks very much for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix nullability of ProjectSnapshot.GetDocument(...)
3 participants