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

expressionUpdates do not contain Method Pointer of operators. #7520

Closed
farmaazon opened this issue Aug 8, 2023 · 9 comments · Fixed by #7659
Closed

expressionUpdates do not contain Method Pointer of operators. #7520

farmaazon opened this issue Aug 8, 2023 · 9 comments · Fixed by #7659
Assignees
Milestone

Comments

@farmaazon
Copy link
Contributor

I have a project, where next to the starting "Press tab to..." node, I added 3 + node.

Then I inspected the network tab and look through "expressionUpdate" messages IDE had received from the engine. No expression contained a method pointer for "+" operator (there was no "+" character in all messages).

3 + _ or even 3 + 4 also had no method pointer. Tested on 2023.2.1-nightly.2023.8.4 engine.

This task is blocking #7107. The IDE implementation is on branch wip/farmaazon/placeholders-for-operators - you can test if your fix makes the argument names appear in nodes like 3 +.

@hubertp
Copy link
Collaborator

hubertp commented Aug 8, 2023

This is slightly confusing since it should have been fixed in #6374 (comment).
And in fact later a separate ticket was created #7190 which blamed IDE (and the #7107 that you mention).

@4e6
Copy link
Contributor

4e6 commented Aug 8, 2023

There are multiple issues.

  1. The expression 3 + 4 does not return the method pointer because there is an issue with the registration of Integer.+ method. Although the method is defined in the Numbers module, it does not get picked up by the compiler

    + : Number -> Number
    + self that = @Builtin_Method "Integer.+"

    The "a" + "b" expression on Text returns the Text.+ method pointer correctly.

  2. The expression "a" + is represented not as a method call but as a closure in runtime. When the method is called with the dot notation "a".+, the expression returns the Text.+ method pointer correctly. I need to check the codegen to see why it generates a closure instead of a method call.
    The expression 3.+ does not return a method pointer because of the issue (1) with the registration of the Integer.+ method.

@4e6 4e6 self-assigned this Aug 8, 2023
@4e6 4e6 removed the triage label Aug 8, 2023
@farmaazon
Copy link
Contributor Author

I propose to fix 2 first, as it allows me to test my branch (I'll test "a" + instead of 3 +)

@hubertp
Copy link
Collaborator

hubertp commented Aug 9, 2023

  1. The expression 3 + 4 does not return the method pointer because there is an issue with the registration of Integer.+ method. Although the method is defined in the Numbers module, it does not get picked up by the compiler
    + : Number -> Number
    + self that = @Builtin_Method "Integer.+"

@4e6 Probably related to #6959. it felt like a hack when I had to introduce this exception, now it feels even more.

@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Aug 15, 2023
@jdunkerley jdunkerley added this to the Beta Release milestone Aug 15, 2023
@hubertp
Copy link
Collaborator

hubertp commented Aug 21, 2023

@Akirathan probably related to our discussion today.

@4e6
Copy link
Contributor

4e6 commented Aug 23, 2023

After the investigation today, it seems like the issue (1) with the registration of Integer.+ method should be fixed by #6959. I'll check once again after it is implemented.

@enso-bot
Copy link

enso-bot bot commented Aug 24, 2023

Dmitry Bushev reports a new STANDUP for yesterday (2023-08-23):

Progress: Started working on the issue. Started looking into the issue with the registration of an Integer.+ method. Created a test case reproducing the example. Discovered that the Small_Integer.+ method is registered instead. This issue should be fixed in 6959 It should be finished by 2023-08-28.

Next Day: Next day I will be working on the #7520 task. Continue working on the task

@enso-bot
Copy link

enso-bot bot commented Aug 24, 2023

Dmitry Bushev reports a new STANDUP for today (2023-08-24):

Progress: Continue working on the issue. Switched to the second part of the issue with the generation of the closure node instead of a method call. Created the test reproducing the issue. Tracked down to the SectionToBinOp compiler pass. Started working on the fix. It should be finished by 2023-08-28.

Next Day: Next day I will be working on the #7520 task. Continue working on the task

@enso-bot
Copy link

enso-bot bot commented Aug 25, 2023

Dmitry Bushev reports a new STANDUP for today (2023-08-25):

Progress: Continue working on the issue. Updated the SectionToBinOp compiler pass to generate a function application instead of a lambda for left sections. Fixed the tests. Created a draft PR. It should be finished by 2023-08-28.

Next Day: Next day I will be working on the #7520 task. Continue working on the task

@4e6 4e6 moved this from 📤 Backlog to 🔧 Implementation in Issues Board Aug 28, 2023
@4e6 4e6 moved this from 🔧 Implementation to 👁️ Code review in Issues Board Aug 28, 2023
@mergify mergify bot closed this as completed in #7659 Sep 15, 2023
mergify bot pushed a commit that referenced this issue Sep 15, 2023
close #7520

Changelog:
- update: SectionsToBinOp compiler pass produces function application for left sections
- refactor: simplify the registration of builtin methods
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Sep 15, 2023
jdunkerley pushed a commit that referenced this issue Sep 15, 2023
close #7520

Changelog:
- update: SectionsToBinOp compiler pass produces function application for left sections
- refactor: simplify the registration of builtin methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants