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

Record type spread follow up questions #6158

Closed
zth opened this issue Apr 15, 2023 · 8 comments
Closed

Record type spread follow up questions #6158

zth opened this issue Apr 15, 2023 · 8 comments
Milestone

Comments

@zth
Copy link
Collaborator

zth commented Apr 15, 2023

From reviewing the code some reflections:

  • The fields from the record being spread are not type checked again, but their type is simply used as-is. (That might explain why the seemingly needed code to add option to the type has no effect).
  • Another possibility is to extract the untyped parse tree for those fields and type check again. Though doing that would preclude the possibility of, in future, know what parts are coming from a spread and what parts are not. It might be useful, or it might not, to be seen.

Now that all the new features exist in isolation, there are a bunch of questions to answer:

  • Are there unboxed records (perhaps with exactly 1 field if I remember)
  • If they exist, can one spread it?
  • Does it become boxed if it's not declared unboxed?
  • If it's boxed, can one make one unboxed out of it?

So it looks like the design is not finished here. We can try to answer some of these questions and iterate some more see what comes out of it.

The question about unboxed apply to type coercion too.
Are we simply allow to coerce a boxed to an unboxed record and the other way round silently? That would not be safe.
What about @as(...)? Are we taking that into account correctly? Both in terms of spread and of type coercions.

Most of these questions can be answered by writing a few little tests.

Link to original conversation: #6154 (comment)

So, questions are (and my personal thoughts next to them):

  • Are there unboxed records? Yes I believe there are, allowing just one field
  • If they exist, can one spread it? I don't think we should allow spreading them, seems like a niche use case that isn't worth complicating things for
  • Does it become boxed if it's not declared unboxed? Can skip if we disallow spreading them
  • If it's boxed, can one make one unboxed out of it? Can skip if we disallow spreading them
  • Are we simply allow to coerce a boxed to an unboxed record and the other way round silently? That would not be safe. What about @as(...)? Are we taking that into account correctly? Both in terms of spread and of type coercions. Maybe we shouldn't allow type coercion on unboxed records either. And for @as, I assume we'd have to account for them in coercion as well, as in @as must exactly match between types for them to be considered equivalent.

My thoughts, cc @cristianoc

@cristianoc
Copy link
Collaborator

In order to test the current status and address the questions and concerns raised in the conversation, I recommend taking the following steps:

  1. Create a comprehensive set of test cases: Develop a series of test cases that cover different scenarios related to unboxed records, spreading, and type coercion. These tests should help answer the questions raised and identify any potential issues or limitations in the current implementation.

  2. Verify the behavior of unboxed records: Test and confirm whether unboxed records with one field are supported and functioning as expected.

  3. Assess the compatibility of spreading with unboxed records: Based on the test cases, evaluate whether spreading should be allowed for unboxed records, considering the potential complexities and use cases.

  4. Evaluate the behavior of boxing and unboxing records: If spreading of unboxed records is disallowed, skip this step. Otherwise, examine how records become boxed or unboxed during the spreading process and determine if this behavior is desirable or if it needs to be modified.

  5. Examine type coercion and the use of @as with unboxed records: Test the safety of coercing boxed and unboxed records, and analyze how the @as operator should be taken into account in terms of spreading and type coercion.

  6. Update the design and implementation based on findings: Iterate on the design and implementation as needed, based on the results of the tests and any new insights gained during the process.

  7. Document the decisions and rationale: Clearly document the decisions made regarding the handling of unboxed records, spreading, and type coercion, as well as the reasons behind those decisions.

  8. Communicate the results and changes: Share the findings, updated design, and any changes made to the implementation with the team or relevant stakeholders. Encourage feedback and discussion to ensure that the proposed solution is robust and addresses the concerns raised in the conversation.

By following these steps, you will be able to test the current status effectively and make informed decisions about the handling of unboxed records, spreading, and type coercion in the codebase.

@cristianoc
Copy link
Collaborator

  1. this test
@unboxed
type t1 = {x:int}
type t2 = {...t1}
let three:t2 = {x:3}


type t3 = {x:int}
@unboxed
type t4 = {...t3}
let seven:t4 = {x:7}

gives:

var three = {
  x: 3
};

var seven = 7;

@zth what do you think?

@cristianoc
Copy link
Collaborator

For type coercion an unboxed, it is indeed problematic at the moment.

This compiles:

@unboxed
type t1 = {x:int}

type t2 = {x:int}

let toT2 = (v:t1) => (v :> t2)
let toT1 = (v:t1) => (v :> t2)

let v1:t1 = {x:42}
let v2 = toT2(v1)
let r = v2.x

and produces

var r = (42).x;

var v1 = 42;

var v2 = 42;

@cristianoc
Copy link
Collaborator

Type coercion and @as is also problematic.

This compiles:

type t1 = {@as("xx") x:int}

type t2 = {@as("xxxxx") x:int}

let toT2 = (v:t1) => (v :> t2)

let v1:t1 = {x:42}
let v2 = toT2(v1)
let eq = v2 == {x:42}

and produces:

var v1 = {
  xx: 42
};

var eq = Caml_obj.equal(v1, {
      xxxxx: 42
    });

var v2 = v1;

@cristianoc
Copy link
Collaborator

Spread and @as works just fine.
So I think this completes all the cases. So we know what the current behaviour is.

@cristianoc cristianoc added this to the v11.0 milestone Apr 15, 2023
@cristianoc
Copy link
Collaborator

The unboxed type coercion is fixed here: c0bd807 (went directly into master by mistake)

@cristianoc
Copy link
Collaborator

With the fixes, I think this is all done.
One can argue about the semantics of spreading unboxed. The current semantics is "copy and paste" semantics.
One would need to either change the semantics, if argued that it should be different, or add extra type errors to prevent the question to arise. Not sure adding type errors adds much value, and it surely adds implementation complexity.
Can be revisited if there are questions.

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

2 participants