There are many parts of this PR, which fix the different parts of this bug. This PR is a draft, because I'd like to try to break out some of these into smaller PRs, which can be reviewed separately; I'm opening this here so that we have a reference branch for all of it together.
The various things in the PR are:
the first part of the bug is the handling of array types when Specimin is manipulating strings that represent type names. All of the improvements are to account for the fact that types may have array brackets, which need to be stripped before e.g., looking up the type in a map. While I was editing these parts of the code, I made similar improvements so that generics are also similarly handled.
some changes to whether fully-qualified or simple names are passed around were required. This is to deal with the fact that in the real example, there are actually two classes named Method: one imported, and another that is only referenced by its FQN. To get this right, we need to keep FQNs around a bit longer when reasoning about method call expressions. Related, I wrote some new logic for splitting up an FQN into the package and class names using the heuristic that packages are named with lowercase and classes are named with uppercase. This is Java convention but isn't actually guaranteed, so I added a note to the README about this.
TargetMethodFinderVisitor did not directly add to the list of used classes the types of the parameters of used methods or constructors. When that is the only place that such a type is used, then they'd have been excluded. This rarely occurs in practice (usually, there is an in-scope variable with the same type at the call site that serves as the argument), but when it does it causes a crash. I added logic to both the handling of method calls and constructor calls to explicitly add the types of each parameter when a method is used.
The logic for adding a used type in TargetMethodFinderVisitor didn't account for arrays, either, so I fixed that as well.
UnsolvedSymbolVisitor did not consider used constructors as "potential used members" (only methods and fields), which caused some logic to not fire for the parameters of used constructors
UnsolvedSymbolVisitor's logic for creating synthetic classes was only triggered for ClassOrInterfaceTypes, but it needs to be triggered for array and wildcard types, too (to create synthetic classes for the component types and bounds, respectively). I factored the logic out and then shared it between the visit methods for all of ArrayType, WildcardType, and ClassOrInterfaceType
when JavaTypeCorrect found a problem that requires a change of type, we have logic in UnsolvedSymbolVisitor to look up the right package for the simple name that javac says that the type should be. However, javac can provide a more complex type than just a simple name, such as an array type or a type containing a generic. In these cases, before looking up the package associated with the type, we need to strip off the type modifiers (the array brackets, etc) before checking the class-to-package map
I spent some time trying to break up this PR and I've decided that it's not worthwhile. @LoiNguyenCS if you can't make sense of it, let me know and I'll try a bit harder. I think it's ready for review.
Fixes #86.
There are many parts of this PR, which fix the different parts of this bug. This PR is a draft, because I'd like to try to break out some of these into smaller PRs, which can be reviewed separately; I'm opening this here so that we have a reference branch for all of it together.
The various things in the PR are:
Method
: one imported, and another that is only referenced by its FQN. To get this right, we need to keep FQNs around a bit longer when reasoning about method call expressions. Related, I wrote some new logic for splitting up an FQN into the package and class names using the heuristic that packages are named with lowercase and classes are named with uppercase. This is Java convention but isn't actually guaranteed, so I added a note to the README about this.TargetMethodFinderVisitor
did not directly add to the list of used classes the types of the parameters of used methods or constructors. When that is the only place that such a type is used, then they'd have been excluded. This rarely occurs in practice (usually, there is an in-scope variable with the same type at the call site that serves as the argument), but when it does it causes a crash. I added logic to both the handling of method calls and constructor calls to explicitly add the types of each parameter when a method is used.TargetMethodFinderVisitor
didn't account for arrays, either, so I fixed that as well.UnsolvedSymbolVisitor
did not consider used constructors as "potential used members" (only methods and fields), which caused some logic to not fire for the parameters of used constructorsUnsolvedSymbolVisitor
's logic for creating synthetic classes was only triggered forClassOrInterfaceType
s, but it needs to be triggered for array and wildcard types, too (to create synthetic classes for the component types and bounds, respectively). I factored the logic out and then shared it between thevisit
methods for all ofArrayType
,WildcardType
, andClassOrInterfaceType
JavaTypeCorrect
found a problem that requires a change of type, we have logic inUnsolvedSymbolVisitor
to look up the right package for the simple name that javac says that the type should be. However, javac can provide a more complex type than just a simple name, such as an array type or a type containing a generic. In these cases, before looking up the package associated with the type, we need to strip off the type modifiers (the array brackets, etc) before checking the class-to-package map