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

Different tear-offs are wrongly identical in CFE #47462

Closed
sgrekhov opened this issue Oct 14, 2021 · 7 comments
Closed

Different tear-offs are wrongly identical in CFE #47462

sgrekhov opened this issue Oct 14, 2021 · 7 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P2 A bug or feature request we're likely to work on

Comments

@sgrekhov
Copy link
Contributor

Consider the following code

typedef MyList<T extends num> = List<T>;

main() {
  var v1 = MyList<num>.filled;
  var v2 = MyList<num>.filled;
  var v3 = (MyList.filled)<num>;

  Expect.identical(v1, v2);
  print(identical(v1, v3));  // true
}

v1 and v3 should not be identical, see #47267

Tested on Dart SDK version: 2.15.0-208.0.dev (dev) (Mon Oct 11 22:12:57 2021 -0700) on "windows_x64"

@a-siva a-siva added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Oct 14, 2021
@a-siva
Copy link
Contributor

a-siva commented Oct 14, 2021

/cc @alexmarkov

@alexmarkov
Copy link
Contributor

Not sure I fully understand why these tear-offs should not be identical. @eernstg Could you clarify?

Anyway, both MyList<num>.filled and (MyList.filled)<num> are evaluated to constants in the kernel file:

  static method main() → dynamic {
    (core::int, core::num, {growable: core::bool}) → core::List<core::num> v1 = #C2;
    (core::int, core::num, {growable: core::bool}) → core::List<core::num> v2 = #C2;
    (core::int, core::num, {growable: core::bool}) → core::List<core::num> v3 = #C4;
    exp::Expect::identical(v1, v2);
    core::print(core::identical(v1, v3));
  }

  ...

constants  {
  #C1 = static-tearoff core::List::filled
  #C2 = instantiation core::List::filled <core::num>
  #C3 = constructor-tearoff core::List::filled
  #C4 = instantiation core::List::filled <core::num>

Although the constants C2 and C4 are different, they are structurally equivalent and canonicalized to the same constant at runtime. If we really want those tear-offs to be different, we should fix CFE so it wouldn't evaluate initializer of v3 to the same constant.
/cc @johnniwinther

@alexmarkov alexmarkov added area-front-end Use area-front-end for front end / CFE / kernel format related issues. and removed area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. labels Oct 14, 2021
@alexmarkov alexmarkov removed their assignment Oct 14, 2021
@eernstg
Copy link
Member

eernstg commented Oct 15, 2021

The constructor tearoff specification relies on a concept of a corresponding constructor function, which is a statically resolved function (it could be a top-level function or a static method, but that's up to the compiler and should not matter). Of course, an implementation could choose to implement this in a way that does not use corresponding constructor functions, but the behavior of identical is specified to be as if we had those functions.

A type alias like MyList has a corresponding constructor function with one type parameter with the bound num for each constructor of the ultimately denoted class List. So if we tear off the generic function MyList.filled then we get a function object which is distinct from the value of the tearoff List.filled (because the latter cannot have that type parameter bound).

This implies that (MyList.filled)<int> must be a generic instantiation of a different function object than List.filled, because there is no way the class List can have a corresponding constructor function which is the same as that of MyList.

Generic instantiations of different function objects should not yield the same resulting function object.

I think the culprit here could be that (MyList.filled)<int> is considered to be a constructor tearoff which has received its actual type arguments (like MyList<int>.filled). For a constructor tearoff that has received its type arguments, the constructor-tearoffs specification requires that the ultimate constructor is resolved, so we expand the type alias and note that Mylist<int>.filled is the same thing as List<int>.filled. Hence identical(MyList<int>.filled, List<int>.filled).

@johnniwinther, does this distinction open a can of worms, or would it be a highly localized change? It looks like a distinction between considering the evaluation of (MyList.filled)<int> as a two-step process or as a one-step process.

@devoncarew
Copy link
Member

devoncarew commented Oct 19, 2021

@johnniwinther @eernstg @leafpetersen - is this a blocking issue for shipping ctor tearoffs? (If so, can you triage to P1, and P2 if not)

@a-siva a-siva changed the title Different tear-offs are wrongly identical in VM Different tear-offs are wrongly identical in CFE Oct 19, 2021
@leafpetersen
Copy link
Member

@lrhn @eernstg do we specify dis-identity here? Do we want to?

@eernstg
Copy link
Member

eernstg commented Oct 19, 2021

Based on the analysis here, I do believe we specify that MyList.filled should evaluate to a function which is distinct from any generic corresponding constructor function associated with List, and (MyList.filled)<int> should then still be different from any generic function instantiation of any of those List constructor function. On the other hand MyList<int>.filled should be resolved to mean the same thing as List<int>.filled, because MyList<int> is expanded to List<int>. So I'd say that they should not be identical.

Let's consider the opposite position. If we choose to say that (MyList.filled)<int> should also be resolved to List<int>.filled (such that the given identical is true) then we might as well evaluate larger expressions similarly: (b ? MyList.filled : YourList.filled)<int>, and so on, and I think that would be difficult to motivate and remember.

@johnniwinther
Copy link
Member

I thought the CFE already treated MyList<num>.filled and (MyList.filled)<num> as different, but apparently now. I'll take a closer look.

@johnniwinther johnniwinther self-assigned this Oct 20, 2021
@johnniwinther johnniwinther added the P2 A bug or feature request we're likely to work on label Oct 20, 2021
Constructor tear-offs automation moved this from To do to Done Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P2 A bug or feature request we're likely to work on
Projects
Development

No branches or pull requests

7 participants