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

Dynamic widget visualization is missing suggestions for values with warnings #6045

Closed
hubertp opened this issue Mar 22, 2023 · 4 comments · Fixed by #6067
Closed

Dynamic widget visualization is missing suggestions for values with warnings #6045

hubertp opened this issue Mar 22, 2023 · 4 comments · Fixed by #6067
Assignees
Labels
--bug Type: bug -compiler p-high Should be completed in the next sprint

Comments

@hubertp
Copy link
Contributor

hubertp commented Mar 22, 2023

Follow up on #5983, .

Notice missing selector when dealing with values that have warnings:
image
image

@hubertp hubertp added p-high Should be completed in the next sprint --bug Type: bug -compiler labels Mar 22, 2023
@hubertp hubertp self-assigned this Mar 22, 2023
@hubertp hubertp moved this from ❓New to 📤 Backlog in Issues Board Mar 22, 2023
@hubertp
Copy link
Contributor Author

hubertp commented Mar 22, 2023

Looks like the culprit is this delegation to a new node as done in InvokeMethodNode and InvokeCallableNode

@hubertp hubertp moved this from 📤 Backlog to 🔧 Implementation in Issues Board Mar 22, 2023
@enso-bot
Copy link

enso-bot bot commented Mar 23, 2023

Hubert Plociniczak reports a new STANDUP for yesterday (2023-03-22):

Progress: The insertion of the new node, when dealing with warnings, leads to a function call node not being replaced by its instrumented version. That in turn later leads to missing setup and lack of caller node. Need to figure out if this is a problem with our code or truffle. It should be finished by 2023-03-24.

Next Day: Next day I will be working on the #6045 task. Investigate why no instrumentation node is being inserted. Look into the new execution context design.

@enso-bot
Copy link

enso-bot bot commented Mar 24, 2023

Hubert Plociniczak reports a new STANDUP for yesterday (2023-03-23):

Progress: Finally managed to figure why instrumentation wasn't behaving as we expected. Turned out that insertion of nodes during runtime would produce nodes (and their children) that were never instrumentable, not only for warnings. This was because the new nodes lacked UUIDs. The actual fix is simple and ready in draft PR. The only thing missing is tests but they are not easy to set up. It should be finished by 2023-03-24.

Next Day: Next day I will be working on the #6045 task. Finish tests and undraft the PR. Spend some time on bookclubs. Pick up next task.

@enso-bot
Copy link

enso-bot bot commented Mar 27, 2023

Hubert Plociniczak reports a new STANDUP for the provided date (2023-03-24):

Progress: Figured out how to set up the test for UUID instrumentation. Had to also deal with separate compilation. It should be finished by 2023-03-24.

Next Day: Next day I will be working on the #6045 task. Scheduling new tasks, testing the current state of LS, bookclub.

@hubertp hubertp moved this from 🔧 Implementation to 👁️ Code review in Issues Board Mar 27, 2023
@mergify mergify bot closed this as completed in #6067 Mar 27, 2023
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Mar 27, 2023
mergify bot pushed a commit that referenced this issue Mar 27, 2023
Instrumentation of calls involving warning values never really worked because:
1) newly created nodes didn't set the UUID of their children
2) the instrumentable wrappers always had an empty (i.e. null) UUID and
they never referred `get`/`setId` calls to their delegates

On the surface, everything worked fine. Except when one actually relied on the instrumentation of values with warnings for proper setup. Then no instrumentation (replacement of nodes) was performed due to empty UUID (as required by `hasTag` of `FunctionCallInstrumentationNode`).

Closes #6045. Discovered in #5893.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug -compiler p-high Should be completed in the next sprint
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant