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

Remove backend for lazy #6960

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DZakh
Copy link
Contributor

@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.

@@ -191,7 +191,7 @@ let rec classify_expression : Typedtree.expression -> sd =
| 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
Contributor 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
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.

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
Contributor 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?

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