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

Unable to access constructor value in interactive mode #8626

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Dec 22, 2023

Pull Request Description

close #7184

The constructor value was not accessible because during the re-compilation a new instance of the type was registered in runtime. Then during the execution, an old cached instance of the type was used in method resolution.

Changelog:

  • update: the registration of types in runtime
  • update: invalidate cached nodes that became a resolution error after applying the edit

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Dec 22, 2023
@4e6 4e6 self-assigned this Dec 22, 2023
@4e6
Copy link
Contributor Author

4e6 commented Dec 22, 2023

Re-using the type in the compiler scope fixed the issue with method resolution. Now the fun part is to invalidate the scope (and the context caches) properly when the type is edited.

val logOut: ByteArrayOutputStream = new ByteArrayOutputStream()
val context =
Context
.newBuilder(LanguageInfo.ID)
Copy link
Member

Choose a reason for hiding this comment

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

It's probably not necessary and maybe even desirable for these tests, for you don't need to limit the polyglot Context to "enso". Now, in runtime, you have access to all the languages and instruments that we use everywhere.

@4e6 4e6 marked this pull request as ready for review December 26, 2023 13:33
@4e6
Copy link
Contributor Author

4e6 commented Dec 26, 2023

I just realized that the update of cache invalidation logic will also fix the scenario when an import got changed making some of the symbols in the program invalid (unresolved). I remember that we have discussed this issue earlier but I was not able to find an open ticket for it.

@4e6 4e6 added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jan 2, 2024
@4e6 4e6 requested a review from Akirathan January 3, 2024 09:03
import java.util.UUID

@scala.annotation.nowarn("msg=multiarg infix syntax")
class RuntimeTypesTest
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure it is worth it but we already have runtime instrumentation tests in Java

@@ -71,8 +71,9 @@ public ModuleScope(
this.exports = exports;
}

public void registerType(Type type) {
types.put(type.getName(), type);
public Type registerType(Type type) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the intended return value of this method? Is it:

  • some Type
  • previous Type
  • newest Type

I am not sure I get what the return type shall represent.

@@ -396,7 +397,6 @@ public void reset() {
imports = new HashSet<>();
exports = new HashSet<>();
methods = new HashMap<>();
types = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

ModuleScope.reset() no longer removes types. OK.

@@ -406,7 +406,7 @@ public void modifyModuleSources(
rope -> {
logger.log(
Level.FINE,
"Applied edits. Source has {} lines, last line has {} characters",
"Applied edits. Source has {0} lines, last line has {1} characters.",
Copy link
Member

Choose a reason for hiding this comment

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

I see. Logging in Java requires {0}.

@@ -345,6 +345,7 @@ public boolean isEigenType() {

public void registerConstructor(AtomConstructor constructor) {
constructors.put(constructor.getName(), constructor);
gettersGenerated = false;
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice, if this was covered by a (non Runtime*Test) unit test

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be adding new Scala code.

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Jan 3, 2024
@mergify mergify bot merged commit cfab344 into develop Jan 3, 2024
35 checks passed
@mergify mergify bot deleted the wip/db/7184-unable-to-access-constructor-value-in-interactive-mode branch January 3, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to access constructor value in interactive mode
4 participants