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

Unexpected Type_Error/Method unknown on project startup #11325

Closed
hubertp opened this issue Oct 15, 2024 · 13 comments · Fixed by #11354
Closed

Unexpected Type_Error/Method unknown on project startup #11325

hubertp opened this issue Oct 15, 2024 · 13 comments · Fixed by #11354
Assignees
Labels
-compiler p-highest Should be completed ASAP

Comments

@hubertp
Copy link
Collaborator

hubertp commented Oct 15, 2024

Encountering regularly in a project a missing method Panic such as:
Screenshot from 2024-10-15 09-56-59

Surprisingly, when we refresh the project, it disappears. Similarly, if (IR) caches for the project are there, the project will also continue to load without hiccups.

@hubertp hubertp added p-highest Should be completed ASAP -compiler labels Oct 15, 2024
@hubertp hubertp self-assigned this Oct 15, 2024
@hubertp hubertp moved this from ❓New to 🔧 Implementation in Issues Board Oct 15, 2024
@hubertp
Copy link
Collaborator Author

hubertp commented Oct 15, 2024

Turns out that In_Any is first type checked with expected Columns_To_Keep type and next with Match_Colums. The later is obviously wrong and leads to failures.

@hubertp
Copy link
Collaborator Author

hubertp commented Oct 15, 2024

Update: looks like the last argument in .union node2 ..In_Any ..By_Name application is actually treated as cached and unresolved constructor ..By_Name is not even resolved. Instead, instrumentation identifies that node as already cached and returns In_Any for ..By_Name, which is clearly wrong.

The reason why instrumented appears to re-use the result is, at it seems, that we index by the context's instrumented node which seems to be the actual application.

@hubertp
Copy link
Collaborator Author

hubertp commented Oct 15, 2024

One more info:
In an application of any1.union node2 ..In_Any ..By_Name we have to resolve UnresolvedConstructor for both arguments using the type information. Arguments/closures of ..In_Any and ..By_Name will both construct an application on the fly https://github.com/enso-org/enso/blob/develop/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/UnresolvedConstructor.java#L173.
The problem is that such applications will get the same UUID, as per search logic. So when ..In_Any is resolved and cached, the next argument re-uses that value from the cache i.e. ..By_Name resolution is skipped and instrumentation immediately returns a (wrong) cached value, Columns_To_Keep.In_Any.

Pinging @JaroslavTulach, as he wrote that logic initially.

Not yet sure, why on refresh it somehow manages to not report type failures.

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

@JaroslavTulach
Copy link
Member

Thanks a lot for the detailed investigation. Yes, we have to improve the search logic.

That means the issue goes towards me (unless you want to have some Truffle fun).

@hubertp
Copy link
Collaborator Author

hubertp commented Oct 16, 2024

That means the issue goes towards me (unless you want to have some Truffle fun).

Don't take all the fun to yourself. I can tackle it.

@enso-bot
Copy link

enso-bot bot commented Oct 16, 2024

Hubert Plociniczak reports a new STANDUP for the provided date (2024-10-14):

Progress: Turned out #11272 is caused by the same issue #11251 after all.To not duplicate the effort moved on to #11325. A hidden type error is wrapped in a Panic which later causes the method not found issue on startup. Unsure yet where the type error is coming from. It should be finished by 2024-10-16.

Next Day: Next day I will be working on the #11325 task. Continue investigating the issue

@enso-bot
Copy link

enso-bot bot commented Oct 16, 2024

Hubert Plociniczak reports a new STANDUP for yesterday (2024-10-15):

Progress: UnresolvedConstructor in autoscoping leads to us building Application node for function argument. That new Application node inherits ID from its parent Application. In fact all such newly created nodes (if there are more autoscoping arguments) inherit ID from the same Application node. This clashes with caching and causes the underlying bug. It should be finished by 2024-10-16.

Next Day: Next day I will be working on the #11325 task. Figure out a unit test and fix the issue.

@hubertp
Copy link
Collaborator Author

hubertp commented Oct 17, 2024

The reason why initially we get the type error and later it disappears is because of... ID maps. Initially, ..In_Any or ..By_Name have no UUID. With empty location it uses the search path to discover the first parent application and then ID/caching conflicts happen, as described above.
Later, ID map will arrive from GUI and type error disappears. Unfortunately it looks like the resulting Panic persists causing method not found problems.

I would claim that even the first type error should not be happen/be displayed to the user as it causes confusion.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Oct 17, 2024

even the first type error should not be happen(ing)

No, the error shall not be happening regardless of UUIDs.

Does the error happen in CLI?
Can it happen in CLI with runtime-execution-instrument enabled?

it uses the search path to discover the first parent application

What is the stacktrace when the path is being discovered? By whom? Who needs it?

@hubertp
Copy link
Collaborator Author

hubertp commented Oct 17, 2024

Does the error happen in CLI? Can it happen in CLI with runtime-execution-instrument enabled?

No, because there are no UUIDs.

it uses the search path to discover the first parent application

What is the stacktrace when the path is being discovered? By whom? Who needs it?

That's something I was hoping you will have the answer since you added that search-via-parent path. I'm guessing you wanted to cache the results? No UUID no caching.

@JaroslavTulach
Copy link
Member

Does the error happen in CLI? Can it happen in CLI with runtime-execution-instrument enabled?

No, because there are no UUIDs.

You can add UUID and used them even from CLI. Demo is in Instrumentor_Spec.enso.

it uses the search path to discover the first parent application

What is the stacktrace when the path is being discovered? By whom? Who needs it?

That's something I was hoping you will have the answer since you added that search-via-parent path.

I can take over and debug the problem. Then I am sure I will see the stacktrace. Or you just paste it here to remind me where is the code we are talking about!

@enso-bot
Copy link

enso-bot bot commented Oct 18, 2024

Hubert Plociniczak reports a new STANDUP for the provided date (2024-10-16):

Progress: Struggled to write a unit test that identifies the problem exactly. In the end, also discovered why type error may dissapear - GUI sends IdMap and then the search path, that infers UUID for autoscoping arguments, works. It should be finished by 2024-10-16.

Next Day: Next day I will be working on the #11325 task. Finish PR with a fix for the corner case.

@enso-bot
Copy link

enso-bot bot commented Oct 18, 2024

Hubert Plociniczak reports a new STANDUP for yesterday (2024-10-17):

Progress: Created a fix that also correctly handles other unit tests. Still investigating if more such corner cases can happen. It should be finished by 2024-10-18.

Next Day: Next day I will be working on the #11325 task. More testing is needed. Address PR review.

@github-project-automation github-project-automation bot moved this from 🔧 Implementation to 🟢 Accepted in Issues Board Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler p-highest Should be completed ASAP
Projects
Status: 🟢 Accepted
Development

Successfully merging a pull request may close this issue.

2 participants