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.

Sign up for GitHub

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

Jump to bottom

Print variant runtime repr in error messages, and fix inclusion check. #6669

Merged
merged 5 commits into from
Jun 14, 2024
Merged

Print variant runtime repr in error messages, and fix inclusion check. #6669

merged 5 commits into from
Jun 14, 2024

Conversation

cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented Mar 7, 2024 •
edited
Loading

  • Print @as(...) as required for variant cases.
  • Print @unboxed in the variant type declaration in the outcome printer.
  • Fix issue where attributes such as @unboxed were printed twice.
  • Fix issue where inconsistency in @unboxed in variant declarations between implementation and interface was not checked, and the type error was missing.

Test:

module M: {
  @unboxed type t = | @as(null) A
} = {
  @unboxed type t = | @as(undefined) A
}

Triggered in error message:

Type declarations do not match:
    @unboxed type t = @as(undefined) A
  is not included in
    @unboxed type t = @as(null) A

@cristianoc cristianoc force-pushed the variant_runtime_repe branch from 98155a1 to 625be5e Compare March 11, 2024 17:28
@cristianoc cristianoc force-pushed the variant_runtime_repe branch from 625be5e to c95f8b8 Compare June 13, 2024 08:13
@cristianoc cristianoc changed the base branch from 11.0_release to master June 13, 2024 08:14
@cristianoc
POC print variant runtime repr
08afc2b 
As in `type t = @as(undefined) A`

Triggered in error message:

```res
Type declarations do not match:
    type t = @as(undefined) A
  is not included in
    type t = @as(null) A
```
@cristianoc cristianoc force-pushed the variant_runtime_repe branch from c95f8b8 to 08afc2b Compare June 13, 2024 08:28
@cristianoc
Print @unboxed for variants and check inclusion correctly.
1ad3482 
- Print `@unboxed` in the variant type declaration in the outcome printer.
- Fix issue where attributes such as `@unboxed` were printed twice.
- Fix issue where inconsistency in `@unboxed` in variant declarations between implementation and interface was not chedked.
@cristianoc
snake case
e60731b
@cristianoc
Copy link
Collaborator Author

Original issue here: rescript-lang/rescript-vscode#922

@cristianoc cristianoc requested review from cknitt and zth June 13, 2024 09:17
@cknitt
Copy link
Member

cknitt commented Jun 13, 2024

This is great! Thanks a lot!

Could you

  • add tests
  • add a CHANGELOG entry
  • remove "POC" from the issue title if this is otherwise ready to be merged

?

@cristianoc
Copy link
Collaborator Author

CI caught a bug in Nullable in core (while building the playground) where the type is missing the @unboxed annotation in the implementation. Cc @zth

@cknitt
Copy link
Member

cknitt commented Jun 13, 2024

Also, when building the playground, the following error occurs in CI:

FAILED: src/Core__Nullable.cmj

  We've found a bug for you!
  /home/runner/work/***-compiler/***-compiler/packages/playground-bundling/node_modules/@***/core/src/Core__Nullable.res:1:1-4:28

  1 │ type t<'a> = Js.Nullable.t<'a> =
  2 │   | Value('a)
  3 │   | @as(null) Null
  4 │   | @as(undefined) Undefined
  5 │ 
  6 │ external null: t<'a> = "#null"

  This variant or record definition does not match that of type
    Js.Nullable.t<'a>
  Their internal representations differ:
  the original definition uses unboxed representation.

@cknitt
Copy link
Member

cknitt commented Jun 13, 2024

Ah sorry, didn't see that you had already commented about that.

@cristianoc cristianoc mentioned this pull request Jun 13, 2024
Merged
@cristianoc cristianoc changed the title POC print variant runtime repr in error messages Print variant runtime repr in error messages, and fix inclusion check. Jun 13, 2024
@cristianoc
Copy link
Collaborator Author

@cknitt @zth CI is happy now

cknitt
cknitt approved these changes Jun 14, 2024
View reviewed changes
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

🎉

@cristianoc cristianoc merged commit 95c52fd into master Jun 14, 2024
19 checks passed
@cristianoc cristianoc deleted the variant_runtime_repe branch June 14, 2024 13:19
cknitt pushed a commit that referenced this pull request Jun 15, 2024
@cristianoc @cknitt
Print variant runtime repr in error messages, and fix inclusion check. (
ac0e1dc 
#6669)

* POC print variant runtime repr

As in `type t = @as(undefined) A`

Triggered in error message:

```res
Type declarations do not match:
    type t = @as(undefined) A
  is not included in
    type t = @as(null) A
```

* Print @unboxed for variants and check inclusion correctly.

- Print `@unboxed` in the variant type declaration in the outcome printer.
- Fix issue where attributes such as `@unboxed` were printed twice.
- Fix issue where inconsistency in `@unboxed` in variant declarations between implementation and interface was not chedked.

* snake case

* Changelog and test.

* Bump rescript-core for playground bundling.
cknitt pushed a commit that referenced this pull request Jun 16, 2024
@cristianoc @cknitt
Print variant runtime repr in error messages, and fix inclusion check. (
940f517 
#6669)

* POC print variant runtime repr

As in `type t = @as(undefined) A`

Triggered in error message:

```res
Type declarations do not match:
    type t = @as(undefined) A
  is not included in
    type t = @as(null) A
```

* Print @unboxed for variants and check inclusion correctly.

- Print `@unboxed` in the variant type declaration in the outcome printer.
- Fix issue where attributes such as `@unboxed` were printed twice.
- Fix issue where inconsistency in `@unboxed` in variant declarations between implementation and interface was not chedked.

* snake case

* Changelog and test.

* Bump rescript-core for playground bundling.
cknitt pushed a commit that referenced this pull request Jun 16, 2024
@cristianoc @cknitt
Print variant runtime repr in error messages, and fix inclusion check. (
bf3879e 
#6669)

* POC print variant runtime repr

As in `type t = @as(undefined) A`

Triggered in error message:

```res
Type declarations do not match:
    type t = @as(undefined) A
  is not included in
    type t = @as(null) A
```

* Print @unboxed for variants and check inclusion correctly.

- Print `@unboxed` in the variant type declaration in the outcome printer.
- Fix issue where attributes such as `@unboxed` were printed twice.
- Fix issue where inconsistency in `@unboxed` in variant declarations between implementation and interface was not chedked.

* snake case

* Changelog and test.

* Bump rescript-core for playground bundling.
cknitt pushed a commit that referenced this pull request Jun 16, 2024
@cristianoc @cknitt
Print variant runtime repr in error messages, and fix inclusion check. (
c148a3d 
#6669)

* POC print variant runtime repr

As in `type t = @as(undefined) A`

Triggered in error message:

```res
Type declarations do not match:
    type t = @as(undefined) A
  is not included in
    type t = @as(null) A
```

* Print @unboxed for variants and check inclusion correctly.

- Print `@unboxed` in the variant type declaration in the outcome printer.
- Fix issue where attributes such as `@unboxed` were printed twice.
- Fix issue where inconsistency in `@unboxed` in variant declarations between implementation and interface was not chedked.

* snake case

* Changelog and test.

* Bump rescript-core for playground bundling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cknitt cknitt cknitt approved these changes

@zth zth Awaiting requested review from zth

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cristianoc @cknitt