-
Notifications
You must be signed in to change notification settings - Fork 4.3k
TextChange.NewText has an unexpected nullable annotation #85746
Copy link
Copy link
Open
Labels
Area-CompilersConcept-APIThis issue involves adding, removing, clarification, or modification of an API.This issue involves adding, removing, clarification, or modification of an API.Concept-Design DebtEngineering Debt, Design Debt, or poor product code qualityEngineering Debt, Design Debt, or poor product code qualityConcept-Null AnnotationsThe issue involves annotating an API for nullable reference typesThe issue involves annotating an API for nullable reference types
Milestone
Description
Activity
Metadata
Metadata
Assignees
Labels
Area-CompilersConcept-APIThis issue involves adding, removing, clarification, or modification of an API.This issue involves adding, removing, clarification, or modification of an API.Concept-Design DebtEngineering Debt, Design Debt, or poor product code qualityEngineering Debt, Design Debt, or poor product code qualityConcept-Null AnnotationsThe issue involves annotating an API for nullable reference typesThe issue involves annotating an API for nullable reference types
This member:
roslyn/src/Compilers/Core/Portable/Text/TextChange.cs
Line 31 in 97b4837
is marked as nullable, but there's no clear indication what we intend by that. The constructor throws if you try to pass null, so it seems like it's only allowing null to handle the case of a default-constructed TextChange. SourceText.WithChanges has some code to handle that being null and treat that as an empty string. (But if the only way you can get this is a default-constructed struct, then what's the point since you're replacing nothing with nothing?)
But some other code has odd asserts that we expect non-null values:
roslyn/src/Compilers/Core/Portable/Text/TextChange.cs
Lines 89 to 93 in 0c1dd5b
But not if you call this other method that should do the same thing!
roslyn/src/Compilers/Core/Portable/Text/TextChangeRangeExtensions.cs
Lines 103 to 106 in 0c1dd5b
If the intent of the nullable annotation was to ensure we're "safe" in regards to default constructed structs, it seems like in practice this didn't result in good safety.