add codes for tricky parameters

[LoiNguyenCS]

No description provided.

[LoiNguyenCS]

Professor,

This is the PR to deal with tricky parameters, which hopefully will fix issue #43.

This one feels a bit like going down a rabbit hole. I know it's usually better to split changes into different PRs, but doing that might take a lot of time for me. To make up for it, I've added more comments to explain things better.

Please take a look. Thank you.

[kelloggm]

That's only true if the types with which the generic is instantiated are all solvable. Consider e.g. `NodeList<Node<Import>>` as an example. In this case, we may need to create synthetic classes for all three of NodeList, Node, and Import. I agree that the synthetic class for NodeList doesn't care about how many levels there are, though. Martin

…

On Thu, Nov 16, 2023, 10:59 AM Loi Nguyen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/org/checkerframework/specimin/UnsolvedSymbolVisitor.java <#46 (comment)>: > - return super.visit(parameter, p); + ResolvedParameterDeclaration resolvedParameter = parameter.resolve(); + // Specimin will update synthetic class based on import statements before working on + // parameters. Why a parameter could be resolved but not described? Specimin creates the + // synthetic class based on the import statements, so if the synthetic class indeed needs a + // placeholder, Specimin will miss it. + try { + resolvedParameter.describeType(); + return super.visit(parameter, p); + } catch (IllegalArgumentException e) { + // following the above explanation, typeName will be in this form path.to.A<path.to.B> + String typeName = parameter.getType().asString(); + @SuppressWarnings( + "signature") // since this type is included among the import statements, we can expect + // that when it is used, it will be in its simple form. + @ClassGetSimpleName String typeToAddPlaceHolder = typeName.substring(0, typeName.indexOf("<")); As I understand, since this is generic type, we don’t really need to care about how deep it can go. For example, if this our unsolved type: ImportDeclaration<List<Map<String, String>> , then a synthetic declaration such as ImportDeclaration<T> should be enough to get the program compuled. — Reply to this email directly, view it on GitHub <#46 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6G5NTUZYULAL2VSV3WSBDYEYZ7NAVCNFSM6AAAAAA7NNVTUOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMZUHAYTCNZUG4> . You are receiving this because your review was requested.Message ID: ***@***.***>

[LoiNguyenCS]

Professor,

I have found a cool visit method of JavaParser that can help us deal with those class types and type variables recursively. That is the visit method for ClassOrInterfaceType. There is a weird bug accompanying this method, so I have to add a check to workaround.

I have also deleted the visit method for import statements. I think it is unsafe to assume that if there is no jar paths, every class imported must be unsolved.

[kelloggm]

Following the API, I'd name the loop variable typeParameterValue rather than typeVariable. (I also suggest changing the associated variables, like typeVariableFullName -> typeParameterValueFullName.)

[kelloggm]

I'm also concerned that this code might be overspecialized to the case where classes that have type parameters are used as parameters. Can you add a test case where the target method also defines the type variable in use, such as:

<T> void foo(List<T> list) { ... }

In particular, I'd like a test like this so that we can be certain that this code won't be triggered (creating a class named "T"!) in that case.

[kelloggm]

No extraneous class is created, but Specimin does crash in this case, because it looks for the class T (i.e., a class with the type variable's name). I'll commit the test on this branch. (I don't think I'll have time to actually fix the bug right now, but maybe I'll do that in a bit.)

[kelloggm]

@LoiNguyenCS since you aren't feeling well, I'm going to address my own comments on this PR

[LoiNguyenCS]

Thank you, Professor. I appreciate it.

[kelloggm]

@LoiNguyenCS I think I've fixed the issues I found with this PR. You should look over my changes; some of them are fairly extensive (I needed to add additional plumbing to keep track of what type variables are in scope at each point). If you don't have any concerns, I think this can be merged.

[kelloggm]

LGTM (I wrote a lot of the code)

[kelloggm]

@LoiNguyenCS I've addressed your comments. Please take another look when you have a chance.

[LoiNguyenCS]

Professor,

This changes look good to me. I think the CI should be able to run successfully after we merge #51.

[kelloggm]

@LoiNguyenCS CI is still failing (Checker Framework is complaining about signature checker errors - I'm guessing a bad interaction with the removal of signature annotations in #51).

[LoiNguyenCS]

Professor, it was a consequence of removing annotations. But it's pretty simple to fix.

[LoiNguyenCS]

Professor,

Somehow the Anonymous test and TypeVarSimple test are failing. I will try to figure out the problems.

[kelloggm]

Regarding the failures in the AnonymousClass test - have you pulled the changes from #52 into this branch? If not, we should do that (that might be what's responsible).

[LoiNguyenCS]

have you pulled the changes from #52 into this branch? If not, we should do that (that might be what's responsible).

Yes, Professor. I have merged newest updates from the main branch into this branch.

[LoiNguyenCS]

I've addressed the bug in the TypeVarSimple test and the AnonymousClass test. The failure in the AnonymousClass test was due to a modification I made on this branch before we discussed the issue of flaky tests. At that time, I reordered the methods in the AnonymousClass class, thinking the issue was related to the visit methods rather than the HashSet.

[kelloggm]

LGTM, thanks