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

Enable nullable reference types #277

Merged
merged 23 commits into from
Aug 16, 2020
Merged

Enable nullable reference types #277

merged 23 commits into from
Aug 16, 2020

Conversation

ygra
Copy link
Contributor

@ygra ygra commented Aug 14, 2020

Long list of commits, but perhaps that makes reviewing a bit easier, as the changes are somewhat scoped.

  • Nullable reference types are enabled for the library and the example project
  • All nullable warnings are errors in the library
  • I've managed to have no remaining islands of #nullable disable and the build succeeds and the example still runs.

There have been a few problematic places, though, that have been annotated with comments, and sometimes pointed out in the commit message. Most of those have been silenced for now with a somewhat minimal set of ! operators.

See #230.

TODO (F):

  • after merge, create an issue about
    • ScriptsAtom.BaseAtom being (supposedly) non-nullable
    • make StyledAtom's constructor parameter atom non-nullable, add asserts
    • make TypedAtom.Atom non-nullable, add asserts
    • UnderlinedAtom.Atom
    • VerticalCenteredAtom.Atom
    • OverUnderBox.ScriptBox
    • PredefinedFormulaParser shouldn't return null? to fix predefinedFormula!.RootAtom! (TexFormulaParser.cs:648)
    • Radical.BaseAtom
    • remove TODO / fix the issue with RowAtom ctor RowAtom(SourceSpan? source, IEnumerable<Atom?> elements)
    • fix an issue with RowAtom(SourceSpan? source, Atom? baseAtom) (see the comment)

ygra added 19 commits August 14, 2020 21:05
The `as` operator has been changed to pattern matching to avoid null checks entirely.
Unconstrained generics make a `!` necessary.
This poses a bit of a problem in the StyledAtom ctor in that an Atom? is eventually passed to RowAtom(source, elements), where elements is supposed to not contain null values at all.
There's a few problems that stem from other parts of the code, e.g. ExtensionChar.Repeat being nullable, but used in CharBox, or OverUnderBox using a nullable ScriptBox as if it were not null.
@ygra
Copy link
Contributor Author

ygra commented Aug 15, 2020

OK, I do see build failures on my end as well, but seemingly only for net452:

Seems like the Nullable NuGet package doesn't seem to work as expected.

error CS0246: The type or namespace name 'NotNullWhenAttribute' could not be found (are you missing a using directive or an assembly reference?)
error CS0246: The type or namespace name 'NotNullWhen' could not be found (are you missing a using directive or an assembly reference?)
error CS0246: The type or namespace name 'NotNullWhenAttribute' could not be found (are you missing a using directive or an assembly reference?)
error CS0246: The type or namespace name 'NotNullWhen' could not be found (are you missing a using directive or an assembly reference?)
error CS0246: The type or namespace name 'NotNullIfNotNullAttribute' could not be found (are you missing a using directive or an assembly reference?)
error CS0246: The type or namespace name 'NotNullIfNotNull' could not be found (are you missing a using directive or an assembly reference?)
1>Done building project "WpfMath_xqner3n1_wpftmp.csproj" -- FAILED.

The build seems to work for netcoreapp3.0, which results in the compiled assembly. Subsequent builds in VS (Ctrl+Shift+B) then tell me "Build successful", as the build system apparently thinks all artifacts are there.

@ygra
Copy link
Contributor Author

ygra commented Aug 15, 2020

Apparently this is the problem: manuelroemer/Nullable#11

I guess I'll probably go with conditional compilation for the respective lines and we'll miss a bit nullability fidelity in net452.

@ForNeVeR ForNeVeR self-requested a review August 15, 2020 07:47
Copy link
Owner

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

Hell of a work you did here, thanks a lot! I've reviewed every single change, and left some comments.

I've decided to move some of the remaining work (see the TODOs I've added to the initial post) to a new issue though.

src/WpfMath/Atoms/BigOperatorAtom.cs Outdated Show resolved Hide resolved
src/WpfMath/Atoms/BigOperatorAtom.cs Outdated Show resolved Hide resolved
src/WpfMath/Atoms/CharAtom.cs Outdated Show resolved Hide resolved
src/WpfMath/Atoms/Radical.cs Outdated Show resolved Hide resolved
src/WpfMath/Atoms/ScriptsAtom.cs Outdated Show resolved Hide resolved
src/WpfMath/TexFormulaParser.cs Outdated Show resolved Hide resolved
src/WpfMath/TexPredefinedFormulaParser.cs Outdated Show resolved Hide resolved
src/WpfMath/TexPredefinedFormulaParser.cs Outdated Show resolved Hide resolved
src/WpfMath/TexSymbolParser.cs Outdated Show resolved Hide resolved
src/WpfMath/Utils/Result.cs Outdated Show resolved Hide resolved
Copy link
Owner

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

Thanks for your work!

@ForNeVeR ForNeVeR mentioned this pull request Aug 16, 2020
14 tasks
@ForNeVeR ForNeVeR merged commit 5b4fd21 into ForNeVeR:master Aug 16, 2020
@ygra
Copy link
Contributor Author

ygra commented Aug 16, 2020

Well, thanks for giving me an opportunity to try such a migration. Still planning to do so at work, albeit on a larger, more confusing code base where the only benefit is that I know it fairly well :-)

And I fear the amount of left over todos is probably normal for old code.

@ygra ygra deleted the nullable branch August 16, 2020 18:16
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.

2 participants