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

Dealing with missing types and symbols #74

Open
cal101 opened this issue Jun 26, 2017 · 8 comments
Open

Dealing with missing types and symbols #74

cal101 opened this issue Jun 26, 2017 · 8 comments

Comments

@cal101
Copy link
Collaborator

cal101 commented Jun 26, 2017

Question

Should walkmod have some kind of propagatable incomplete or error symbol and type?

The problem

Walkmod does have a "try your best and continue on error" philosophy.
When it comes to missing symbols that currently means "continue with next compilation unit".
That could be improved.

I am aware of 2 cases where missing symbols happen in operational code:

  1. When code is disabled via "if(false)" either no or incomplete (?) class files are generated for anonymous classes.
  2. Walkmod typically resolves more constructors, fields, methods and types of thirdparty libraries than operational code needs at runtime. That results in "NoClassDefFoundError" on analysis.

The simple variants of case 1 were fixed by pull requests that ignore missing anonymous classes in disabled code and use specially marked SymbolType.
When anonymous classes have fields and extra methods or other members i discovered recently that the marked symbols have to be considered to ignore more otherwise fatal cases.
This is related to case 2 above where symbols of thirdparty types can't be resolved if some class is not contained in the runtime jars of the project because it't not needed or it's optional.

I would like to support a form of continue-on-missing-class strategy here to.
The idea is to ignore all or a list of missing classes by configuration and introduce an "error" symboltype. The error symboltype is treated as a special case that can propagate if used for resolving fields or methods.
This introduces a new category of visitors: visitors need to signal awareness of incomplete symbols to avoid accessing class, method or field data.
So far this allowed me some more simple refactorings than before.
But otherwise this causes rippling changes.
Code and configuration has to request the new behavior so nothing should change for existing code.
What do you think? Is that some useful direction?

@rpau
Copy link
Owner

rpau commented Jun 26, 2017 via email

@cal101
Copy link
Collaborator Author

cal101 commented Jun 26, 2017

I do not think that null is a good choice here.
Too often we had cases where a symbol was missing because of some bug. That should be clearly separated from cases where some class not found when lookjng up a field or constructor because a dependant is missing. The symbol resolution in nested scopes will be different for those cases.
For a lot of binary cases i don't mind if it's nullable or functional style.
But this is ternary style or exceptional case style. The unknown character of sql null does fit here as does the NaN of IEEE floating point. It widens the domain of functions and allows functions to be combined and increases the chance to get useful results if the "unknown parts" of some expression are not needed for obtaining some result.
I fixed a few bugs lately where a usage info of "null" in walkmod symbols was interpreted as "no usage" where it really meant "not initialized" for some reason.
IMHO there is an important difference between having no symbol in some scope and just having a symbol but not being able to lookup it's constructor. I went down the "null" road for a few hours but triggered too many NPE and "Oops" cases where i could not separate the internal errors from the "incomplete type" cases.
The null value just does not carry enough information here.
And I don't think writing more code with "!=" null style helps in the sematic analysis code. It too often just obscures the real causes of the problem.
Some late "oops, missing symbol" may have been detected earlier at the places where null symboldata is written to the AST. But I'm still not familiar enough with the flow to be sure enough to throw in those cases. Another overload on the meaning of null hurts.
If we consider it useful to detect a case we need to store enough of the information if we want to make use of it.

@cal101
Copy link
Collaborator Author

cal101 commented Jun 26, 2017

To make it clear: I don't opt for replacing all exceptions with error symbols. I added at few places where a field lookup or a method or constructor lookup or some class lookup lead to a noclassdefounderror typically for some other class then the target class. The error symbol made it possible to carry on, pass some oops checks and let my refactor visitor do some useful work because the missing symbols were not needed for it's operation.

@rpau
Copy link
Owner

rpau commented Jun 27, 2017 via email

@cal101
Copy link
Collaborator Author

cal101 commented Jun 27, 2017

The unreachable anonymous symbol types are already marked. See Marker enum in Symboltype. Improvements for naming are always welcome.
That is "case 1" in the original post.
But that unreachable symbol type will be used by walkmod e.g. when looking up a field of that anoymous class. I have that case but wasn't able to make a test case yet. I just realized recently that the strange class verify error i get may be related to a field definition of a disabled anonymous class.
I want to give that field a symboltype, too and find null to be lacking information.
I want to keep the "oops" checking for bug detection.
This is a different error condition than disabled anonymous type even if induced by that.
I agree that proper naming is very important here. It's at the heart of the analysis. You have to enable the behavior explicitly and visitors have to tell awareness. For other visitors that need full semantic analysis it should be fatal as before.
"IncompleteType", "UnresolvedType", "UnresolvedDependantType" point in the right direction but are not good enough.

@rpau
Copy link
Owner

rpau commented Jun 27, 2017 via email

@cal101
Copy link
Collaborator Author

cal101 commented Jun 28, 2017

"AnonymousMemberType" does not fit. Too broad, the problem is not that it's an anonymous member type, but a member of a "disabled anonymous class". Probably only a "declared member of disabled anonymous class".
And binding it to anonymous classes related "case 1" only would not allow reuse for other cases like "case 2".

ReplacementType
SubstitutionType
SubstituteType
StopgapType

My favorite: SurrogateType

Stands out and tells "its not the real thing"

@rpau
Copy link
Owner

rpau commented Jun 29, 2017 via email

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

No branches or pull requests

2 participants