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

Regression: Superfluous TranslationUnits #136

Closed
JulianSchuette opened this issue Jul 2, 2020 · 4 comments
Closed

Regression: Superfluous TranslationUnits #136

JulianSchuette opened this issue Jul 2, 2020 · 4 comments
Labels
bug Something isn't working

Comments

@JulianSchuette
Copy link
Contributor

With the revamped type system after version 1.4.2, a few things have changed which break the API/assumptions:

Attached to the new "implicit" RecordDeclaration nodes (see bug #135) is an unknown declarations TranslationUnit. It obviously does not refer to any existing file (TranslationUnit) and provides no value.

For clients of CPG, this complicates things as it breaks the assumption that every TranslationUnit refers to an existing (analyzed) file.

Is that change intended and what is it good for? How are clients expected to handle TransationUnits that do not refer to actual files?

recorddecl

@JulianSchuette JulianSchuette added the bug Something isn't working label Jul 2, 2020
@Masrepus
Copy link
Contributor

Masrepus commented Jul 2, 2020

This unknown declarations TU has been there long before the type system update, it just wasn't that visible, as it only exists for classes that are not present in the analysed files, like java.lang.Object. In order to reach the behavior described in #135, that we have record declarations for unknown classes, we need some place to store them, in order to be able to reach them from the graph root. Our current graph roots are translation units, thus the unknown declarations TU as a container for dummy records

@JulianSchuette
Copy link
Contributor Author

I understand. In 1.4.2 this was not present yet. The thing is - it makes CPG very unpleasant to use and complicates client code. A client now cannot simply assume that a RecordDeclaration is actually present in code and it cannot assume that a TranslationUnit actually exists.

This requires the client to a) know about the different forms and shapes in which a RecordDeclaration may appear and b) write complicated if-then constructs or clunky Gremlin queries than handle the different cases.
Also the assumptions about fields break - for example, a client cannot assume anymore that every RecordDeclaration has a non-null location. This requires changes throughout whole Codyze to check for nullness.

My suggestions would be to represent supertypes as Type nodes rather than RecordDeclaration. So, java.lang.Object becomes a Type that is referenced by the RecordDeclaration Botan in this case. This would also remove the need to have the unknown declarations TranslationUnit.

@Masrepus
Copy link
Contributor

Masrepus commented Jul 2, 2020

This would only remove a single instance of unknown declarations. Even if supertypes are only present as types, any situation where a method is called or statically imported from a class that is unknown, dummies are still created.
I would suggest to rather fix the nullness issues, e.g. set an unknown record's location to a proper unknown one. Concerning the problems with record declarations and translation units not being present in the code, I don't really see where this could be problematic. It has the same effect as just an empty file or an empty class. Wouldn't codyze just skip over those?

@Masrepus
Copy link
Contributor

Masrepus commented Sep 8, 2020

Closing this as we fixed the dummy creation behavior already, thus no unknown declarations TUs are created anymore

@Masrepus Masrepus closed this as completed Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants