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

DoesNotReturnAttribute should affect definite assignment #2555

Closed
YairHalberstadt opened this issue May 23, 2019 · 7 comments
Closed

DoesNotReturnAttribute should affect definite assignment #2555

YairHalberstadt opened this issue May 23, 2019 · 7 comments

Comments

@YairHalberstadt
Copy link
Contributor

YairHalberstadt commented May 23, 2019

The compiler is introducing a new attribute DoesNotReturn that will affect nullability analysis.

I think it would make sense for the compiler to enforce this strictly, and for it to affect definite assignment as well.

It is possible that someone using a different .NET language/old compiler could add this attribute to a method which does in fact return. However this will only lead to undefined behaviour, not unsafe behaviour, because the C# compiler zeroes out the stack before calling a method. I believe that the benefits therefore outway the risk.

See also: #538

@YairHalberstadt YairHalberstadt changed the title Does not return should affect nullability definite assignment DoesNotReturnAttribute should affect definite assignment May 23, 2019
@Joe4evr
Copy link
Contributor

Joe4evr commented May 23, 2019

Also related: #739

@333fred
Copy link
Member

333fred commented May 24, 2019

When we talked about this attribute during CoreFX design, we were pretty explicit that an attribute alone should not affect code generation and definite analysis in this manner. If we ever want to go down the route of affecting definite assignment, we will need to introduce new syntax, likely a conditional keyword such as never or noreturn. That could add this attribute, but it will likely also be accompanied by a modreq to prevent downlevel breaks.

@YairHalberstadt
Copy link
Contributor Author

Then wouldn't now be the right time to add this feature, rather than have two equivalent syntaxes? Or is it too close to C# 8 to do this?

@333fred
Copy link
Member

333fred commented May 24, 2019

To be clear, this wouldn't be two equivalent syntaxes. One would likely supersede the other, with the explicit never syntax allowing the compiler to enforce the condition in the method body. The attribute alone will never affect code in this manner.

As to the timing, yes, I think we're likely too close to C# 8 to make such a change.

@jnm2
Copy link
Contributor

jnm2 commented May 24, 2019

I believe this is where the conversation started during the API review livestream: https://youtu.be/zAhOxraBsi0?t=3665 (almost 20 min long)

@YairHalberstadt
Copy link
Contributor Author

Closing in favour of #538, as this isn't planned in the C# 8 timeframe anyway.

@john-h-k
Copy link

Closed, but thought it was worth mentioning for anyone else who sees this, the addition of SkipLocalsInitAttribute could cause issues with garbage data being returned as the stack isn't zeroed

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

No branches or pull requests

5 participants