-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Manually update C# compiler version to a more recent preview #67440
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
@jaredpar, not sure who to ask about this:
|
@333fred PTAL |
Is it surprising that |
The break was probably introduced by dotnet/roslyn#59750. I'll take a look. |
@stephentoub any idea? it's just a call straight into the runtime. maybe the stack is being mis printed. |
Was talking with @AlekseyTs around generic math and the other issue that I hit (#67440 (comment)). He suggested we should be able to work around both of these issues, if desired. For the params issue it should only occur when the named arguments are supplied out of order. For the ambiguity, we should be able to explicitly pick the right overload with a cast or No preference on what's done here, just relaying what was suggested to me. If we think the fixes will take longer than the P4 snap, then I might have a preference towards doing the workaround so we can get some additional bits in for generic math with that preview. |
It's not calling into other code. Roslyn is calling Environment.FailFast(exception.ToString(), exception), i.e. it's using exception.ToString() as the message, so you're seeing the stack of the exception as the message FailFast prints followed by the stack of where FailFast is being called. |
Yes. I was waiting for Rikki's fix, but I don't want to wait for a design change around string literal overload resolution, as that won't be a quick turnaround. |
I'll workaround the overload issue (particularly since its test impacting only for our current code, afaict) locally then and will wait for Rikki's fix to flow through this PR 👍 |
Thanks for the offer, but I already started in this branch; I'll push that up soon. |
6217592
to
1c68cd7
Compare
@bartonjs, FYI, there were some src changes here in System.Security.Cryptography. At first it seemed like it might actually be more efficient to use the |
1c68cd7
to
d96ddb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM and work on my local branch for user-defined checked operators
.
Both failures look to be already known looking at them compared to the issues the bot linked |
Yeah, I am, unsurprisingly, not pleased by the implicit conversion from a string literal to |
No description provided.