Skip to content

Conversation

@DZakh
Copy link
Member

@DZakh DZakh commented Aug 17, 2024

ParseTree is still there to be removed

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Is this code all dead? So pure refactoring?
Would be good to clarify in the PR description.

| Texp_ident _ | Texp_for _ | Texp_constant _ | Texp_new _ | Texp_instvar _
| Texp_tuple _ | Texp_array _ | Texp_construct _ | Texp_variant _
| Texp_record _ | Texp_setfield _ | Texp_while _ | Texp_setinstvar _
| Texp_pack _ | Texp_object _ | Texp_function _ | Texp_lazy _
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't remove an item or the entire runtime representation is reindexed and the editor extension will crash.
Instead you can give it a unit payload and add a comment that it is unused.

Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we should rename the unused constructors to something like Texp_unused_lazy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure

Copy link
Member Author

Choose a reason for hiding this comment

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

You can't remove an item or the entire runtime representation is reindexed and the editor extension will crash.
Instead you can give it a unit payload and add a comment that it is unused.

Could you explain how removing something from runtime related to the editor extension? I asked the same question in another PR, feel free to answer in a single place.

#6957 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

You can't remove an item or the entire runtime representation is reindexed and the editor extension will crash.
Instead you can give it a unit payload and add a comment that it is unused.

Could you explain how removing something from runtime related to the editor extension? I asked the same question in another PR, feel free to answer in a single place.

#6957 (comment)

The editor extension and the compiler operates on the same generated compiler artifacts. If we change the representation in these artifacts (which happens if we change the types enough) then things stop working.

In the future we'll have tools bound to a specific ReScript version so this isn't a problem. But right now that's not how it works, so we need to take that into account.

Copy link
Member Author

Choose a reason for hiding this comment

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

The editor extension and the compiler operates on the same generated compiler artifacts.

The ones we have in printsmth files? So we can change the payload, but the variant should stay, so the print logic stays the same, is it correct?

  | Texp_lazy (e) ->
      line i ppf "Texp_lazy";
      expression i ppf e;

Is it safe to add new items to the variant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To the payload, or add a new variant case at the end?

@cknitt
Copy link
Member

cknitt commented May 18, 2025

Superseded by #7474

@cknitt cknitt closed this May 18, 2025
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

Successfully merging this pull request may close these issues.

4 participants