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

[Compilers] Generated IL for is check against constant #20642

Closed
Therzok opened this Issue Jul 5, 2017 · 5 comments

Comments

@Therzok
Contributor

Therzok commented Jul 5, 2017

Version Used:
d8c89de

Steps to Reproduce:

  1. Check the code/IL gen here https://gist.github.com/Therzok/6562398fb9a1fe87b407bda9d1c6a3db
  2. Or here: https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxASwDYB8ACAGAAhwEYBuAWACh8jiA6AJQFcA7GNMOOgSTcQHsADgGVEANzQBjOFAqUqOAEwEAshDQsAwhmhQqAbyoECk/i1gENMAmACep89YC8BAKxzjsCO0lEALKrqLAAUJHgA2gC6BBAIAOZQAJRGBIaUAJDpVgQAHgQuAMxymWgAZgTBeWhQNvZmsImpVFSZAL4p7ZSdQA=
  3. See that the generated IL is inefficient

Expected Behavior:
If a value type implements IEquatable, I'd assume that IEquatable<T>.Equals (T other) would be used on the left expression with the right expression as a parameter. This could be done via EqualityComparer<T>.Default.Equals (T a, T b)

Actual Behavior:
object.Equals(object, object) is being used, which leads to inefficient code when using value types.

@gafter

This comment has been minimized.

Member

gafter commented Jul 5, 2017

This scenario doesn't look very realistic. Why would the user's source code use pattern-matching here instead of ==? And why should the generated code use either of those methods when it could use a single IL equality instruction as if you had written ==?

@gafter

This comment has been minimized.

Member

gafter commented Jul 5, 2017

Duplicate of #12813

@gafter gafter closed this Jul 5, 2017

@Therzok

This comment has been minimized.

Contributor

Therzok commented Jul 5, 2017

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/is

I came to this example by looking at docs for the is keyword. Albeit it explicitly states object.Equals is used, not sure why it's shown as an example. Possibly this should be filed under docs, if it is so by design.

@gafter

This comment has been minimized.

Member

gafter commented Jul 5, 2017

The spec does say

The constant [sic] expression is evaluated as follows:

  • If expr and constant are integral types, the C# equality operator determines whether the expression returns true (that is, whether expr == constant).
  • Otherwise, the value of the expression is determined by a call to the static Object.Equals(expr, constant) [sic] method.

Two errors here: first, it isn't a constant expression, it is a pattern-matching expression.

Second, we do the operand of Object.Equals in the opposite order.

This bug report is that we fail to do the first bullet correctly. Reopening.

@gafter gafter reopened this Jul 5, 2017

@gafter gafter added this to the 16.0 milestone Jul 5, 2017

@gafter gafter self-assigned this Jul 5, 2017

@gafter gafter added this to VS 15.5 in Compiler: Gafter Aug 25, 2017

@gafter gafter modified the milestones: 16.0, 15.later Sep 26, 2017

@gafter gafter assigned TyOverby and unassigned gafter Sep 26, 2017

@gafter gafter removed this from VS 15.5 in Compiler: Gafter Sep 29, 2017

@gafter gafter added this to Code Quality in Compiler: Pattern-Matching Nov 17, 2017

@jcouv jcouv assigned gafter and unassigned TyOverby Nov 24, 2017

@jcouv jcouv modified the milestones: 15.6, 16.0 Nov 24, 2017

@gafter

This comment has been minimized.

Member

gafter commented Mar 18, 2018

Closing as a dup of #12813

@gafter gafter closed this Mar 18, 2018

@gafter gafter removed this from Code Quality in Compiler: Pattern-Matching Mar 18, 2018

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