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

Skip importing same class twice (fixes #1463) #1676

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rPraml
Copy link
Contributor

@rPraml rPraml commented Oct 4, 2024

No description provided.

@rPraml
Copy link
Contributor Author

rPraml commented Oct 4, 2024

See #1463 - I would like to have a testcase for this situation ;)

@gbrail
Copy link
Collaborator

gbrail commented Oct 7, 2024

So, do you have an idea how to test? Or do we feel that existing tests cover this well enough?

@rPraml
Copy link
Contributor Author

rPraml commented Oct 8, 2024

I found one case, where I can trigger that error:

importPackage(java.util);
UUID.randomUUID(); // calls getPkgProperty("UUID", global, false)
importClass(java.util.UUID);
UUID.randomUUID();

While the first access calls getPkgProperty("UUID", global, false), the second one calls calls getPkgProperty("UUID",nativeJavaPackage, true)
We could skip this, if we would follow the parent chain here:

synchronized Object getPkgProperty(String name, Scriptable start, boolean createPkg) {
        Object cached = super.get(name, start);
        if (cached != NOT_FOUND) return cached;
        // TODO: check start.getParentScope()

I doubt, that the above use case is the same case as @Easy65 had, as this code fails also in 1.7.11.
I do not have much objections, to use equals instead of ==, but it would be nice, to understand what was the breaking change. (yes #826 is very suspicious)

@rPraml
Copy link
Contributor Author

rPraml commented Oct 8, 2024

Sorry, I've run out of ideas on how to write a test case that shows the regression between 1.7.11 and the current version. I tried different things, shared scopes etc., but either the test fails under both 1.7.11 and current version or passes under both versions.

If no further input comes in, then I would merge this, as it is an improvement.

@Easy65
Copy link

Easy65 commented Oct 8, 2024

Hi, I could finally provide a test, see my latest comment in the issue.
Regards,
Andreas

@rPraml
Copy link
Contributor Author

rPraml commented Oct 8, 2024

Thanks for the test. I'll try to convert this to a unittest the next days

@rPraml rPraml marked this pull request as draft October 8, 2024 11:20
@@ -40,12 +41,10 @@ public void setUp() throws Exception {
}

ctx = Context.enter();
scope1 = ctx.newObject(sharedScope);
scope1 = new NativeObject();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done too complex

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.

3 participants