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

Warn if we're offering a Coalesce Expression inside an ExpressionOfT. #17264

Merged
merged 7 commits into from
Feb 23, 2017

Conversation

CyrusNajmabadi
Copy link
Member

Mitigation for #17028

@CyrusNajmabadi
Copy link
Member Author

The change from x != null ? x : y into x ?? y can be observed inside an Expression<T> context. i.e. a different type of node is generated for ?? vs ? : . Some Expression<T> providers might not support the ?? node. As such, we warn that this may cause problems, but we do not prevent people from making this change if it is what they want to do.

@CyrusNajmabadi
Copy link
Member Author

Tagging @dotnet/roslyn-ide

@CyrusNajmabadi
Copy link
Member Author

Tagging @dotnet/roslyn-ide

@miloush
Copy link

miloush commented Feb 21, 2017

I disagree with this fix. Why warning for the coalesce code fix only? Expression consumers can lack support for arbitrary set of expressions, and any fix potentially changes the behavior.

@miloush
Copy link

miloush commented Feb 21, 2017

I mean it's not even about supporting the expressions or not. The consumer might support them all, but not be executing them. Could be serializing it or hashing it or whatever and things might break. EDIT: The code fixes work under the assumption of C# spec and execution equivalence. If this should be fixed on Roslyn side then I think all code fixes inside ExpressionOfT should be with warning at least.

@CyrusNajmabadi
Copy link
Member Author

Why warning for the coalesce code fix only?

There is no claim that the warning would only be for coalesce expressions. We're just happening to start with that. I'm happy to add more warnings in the future for other features as well.

@CyrusNajmabadi
Copy link
Member Author

If this should be fixed on Roslyn side then I think all code fixes inside ExpressionOfT should be with warning at least.

That's fine. We can do that.

@DustinCampbell
Copy link
Member

If this should be fixed on Roslyn side then I think all code fixes inside ExpressionOfT should be with warning at least.

That's fine. We can do that.

That's fair.

@Pilchie Pilchie added this to the 2.1 milestone Feb 22, 2017
@CyrusNajmabadi CyrusNajmabadi merged commit b176001 into dotnet:master Feb 23, 2017
@CyrusNajmabadi CyrusNajmabadi deleted the warnInsideExpression branch February 23, 2017 23:20
Copy link
Member

@Pilchie Pilchie left a comment

Choose a reason for hiding this comment

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

👍

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

Successfully merging this pull request may close these issues.

6 participants