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

Handle implicit calls from interface or superclass #119

Merged
merged 16 commits into from
Feb 16, 2024

Conversation

LoiNguyenCS
Copy link
Collaborator

Professor,

This is the PR to create synthetic interfaces and superclasses when methods from them are implicitly called.

Please take a look. Thank you.

@LoiNguyenCS
Copy link
Collaborator Author

LoiNguyenCS commented Feb 14, 2024

The CI fails at the evaluation script step. I am not sure why. This discussion in this GitHub issue seems to be suggesting that we have reached the CPU limitations.

@kelloggm
Copy link
Collaborator

The CI fails at the evaluation script step. I am not sure why. This discussion in this GitHub actions/runner-images#6680 seems to be suggesting that we have reached the CPU limitations.

This is almost certainly a resource limitation issue on GitHub's side: our job is using too many resources, so it is being cancelled. We need to either reduce the amount of work that Specimin does, parallelize this job, or split up the evaluation job into multiple parts.

@LoiNguyenCS I know you've said you have some ideas about improving Specimin's performance by reducing the amount of I/O that it does. I think this problem is a good incentive to spending an hour or two investigating whether there is an easy win there.

@tahiat have you made any progress looking into parallelizing the evaluation of these builds using pash, as we discussed a few weeks ago? If we can land that, it would probably fix this problem, too.

@tahiat
Copy link
Collaborator

tahiat commented Feb 14, 2024

Professor, let me look into parallelizing using Pash today. I have not made any progress on that task.

@LoiNguyenCS
Copy link
Collaborator Author

Professor,

I have updated the code based on your review. Please take a look. Thank you.

@LoiNguyenCS
Copy link
Collaborator Author

Professor,

I have updated the codes. Please take a look. Thank you.

Copy link
Collaborator

@kelloggm kelloggm left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks

@LoiNguyenCS
Copy link
Collaborator Author

Professor,

Should we merge this PR or wait until we get the CI failure solved?

@kelloggm
Copy link
Collaborator

Should we merge this PR or wait until we get the CI failure solved?

Let's disable the evaluation script in CI (in this PR is fine) so that CI stays green.

@LoiNguyenCS
Copy link
Collaborator Author

I have temporarily disabled the evaluation script.

@kelloggm kelloggm merged commit ae1d2ca into njit-jerse:main Feb 16, 2024
1 check passed
@kelloggm kelloggm deleted the synthetic-interface branch February 16, 2024 19:03
}
}
if (methodDeclaredTypesOfParameters.equals(methodTypesOfParameters)) {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@LoiNguyenCS Sorry to bring this up now, when this has already been merged, but I don't think that this logic is correct. In particular, it fails to take into account subtyping: it is legal to call a method with arguments that are subtypes of the declared types of the method, but this method will return false in that case. For example, consider:

class A {}
class B extends A {}

class C {
   void test(A a) { ... }
...
   test(new B());
}

The call to test(new B()) is legal and typechecks, but this method would miss that this call is to a method defined in the class. The problem is that the argument and parameter lists don't exactly match up - each argument is a subtype.

I'm unsure if this is causing any of our problems (I observed it while debugging #152, but it is not the proximate cause of that issue I don't think), but we need to come up with a strategy to handle this case properly.

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