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

Remove MaybeNull from Output/Input.Create to avoid spurious warnings #6600

Merged
merged 2 commits into from
Mar 23, 2021

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Mar 22, 2021

Addresses #6295

@t0yv0 t0yv0 linked an issue Mar 22, 2021 that may be closed by this pull request
@t0yv0 t0yv0 added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Mar 22, 2021
@t0yv0
Copy link
Member Author

t0yv0 commented Mar 22, 2021

Let me check if the user can promote null to Output before and after.

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 22, 2021

Looks like the compiler rejects an attempt to pass null into Output.Create both before and after the change, so this is a no-op in that sense.

@mikhailshilkov
Copy link
Member

Looks like the compiler rejects an attempt to pass null into Output.Create both before and after the change

Hmm, how so? Output.Create((string?)null); is valid code, AFAIK.

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 22, 2021

	var mout = Output.Create<string>((string?)null);
	// warning CS8625: Cannot convert null literal to non-nullable reference type. 

It's valid for compilation but does generate a warning. I believe this warning stays before and after the PR.

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 22, 2021

And

	string? boom = null;
	var mout = Output.Create<string>(boom);

	// warning CS8604: Possible null reference argument for parameter 'value' in 'Output<string> Output.Create<string>(string value)'

@mikhailshilkov
Copy link
Member

var mout = Output.Create<string>((string?)null);

Are you missing ? in the type? var mout = Output.Create((string?)null); gives me no warnings (and it shouldn't, it's a valid thing to do).

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 22, 2021

Ah, right, so that's the same as

	Output<string?> mout = Output.Create((string?)null);

right?

This gives me no warnings.

@mikhailshilkov
Copy link
Member

Okay. Next question - why did we have MaybeNull in the first place? I'd love to understand what it solved and why we don't need this anymore.

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 22, 2021

#3399 introduced it. Any ideas?

Copy link
Member

@mikhailshilkov mikhailshilkov left a comment

Choose a reason for hiding this comment

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

The best hypothesis I have is that the intention was to use AllowNull in those cases instead of MaybeNull. MaybeNull seems to be intended for return types, so our usage doesn't look valid. I don't quite understand when AllowNull would be needed - I'd appreciate an explanation if you have it, but otherwise the change LGTM.

@mikhailshilkov
Copy link
Member

Btw, I'd still change-log this fix as the consequence is externally visible.

@mikhailshilkov mikhailshilkov removed the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Mar 23, 2021
@t0yv0
Copy link
Member Author

t0yv0 commented Mar 23, 2021

OK I dug into the AllowNull attribute a little bit. This attribute lets users to defeat the null-checker when "we know what we are doing". Specifically consider the following program that currently fails on Pulumi:

string? foo = null;
Output<string> mout = Output.Create<string>(foo);
/// error CS8604: Possible null reference argument for parameter 'value' in 'Output<string> Output.Create<string>(string value)'

IN current version of Pulumi we would expect the user to fix this to:

string? foo = null;
Output<string?> mout = Output.Create<string?>(foo);

However, if we wanted to accept the original program, and say generate an output that resolves to null although it promises to be Output with a non-nullable string, we could use AllowNull attribute. The AllowNull needs to propagate deeper into the SDK, here is a demonstration of how that might look like:

#6601

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 23, 2021

Now, I have some FP/F# bias so in my book that sounds like a bad idea to add AllowNull. I would vote for default behavior here. If necessary the user can defeat the null checker outside of the SDK - in the user code.

@t0yv0 t0yv0 changed the title Remove MaybeNull attr from read-only params on Input/Output constructors Remove MaybeNull from Output/Input.Create to avoid spurious warnings Mar 23, 2021
@t0yv0 t0yv0 merged commit 980c50c into master Mar 23, 2021
@t0yv0 t0yv0 deleted the fix/6295 branch March 23, 2021 23:34
abhinav pushed a commit to pulumi/pulumi-dotnet that referenced this pull request Jan 11, 2023
…ulumi/pulumi#6600)

* Remove MaybeNull attr from read-only params on Input/Output constructors

* Update changelog
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.

MaybeNull attribute should not be applied to parameters
2 participants