Skip to content

work around scala bug 4762 #205

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

Merged
merged 3 commits into from
Feb 20, 2023
Merged

work around scala bug 4762 #205

merged 3 commits into from
Feb 20, 2023

Conversation

bbrehm
Copy link
Contributor

@bbrehm bbrehm commented Feb 20, 2023

cf scala/bug#4762 and the benchmarks in joernio/flatgraph#11

Generally, I think we need a custom linter rule that forbids this kind of shadowing of superclass fields.

Indeed, my preferred solution to 4762 would be to forbid the compiler from ever silently generating fields from class parameters in non-anonymous classes (i.e. outside of closures).

In other words, class foo(x:Int){def getX:Int = x} would become a warning and would demand a rewrite to class foo(private val x:Int) {def getX:Int = x}.

The general idea of "muh muh the compiler will figure out what fields you need" is imo a braindead stupid decision by the scala language. The single most important fact of a data type, i.e. non-singleton class, is its memory layout, and making it non-trivial to predict the set of fields from a class definition is just painful. Non-triviality is evidenced by the fact that we have wasted 8 bytes per node for god knows how long.

In this specific case, I think the issue is that the read of id in def product refers to the scala class parameter, while all other ones refer to the base class id field. Hence, the derived (duplicate) id field needs to be kept around for def product. Now both are final; so I can't tell whether the scala compiler is too stupid/buggy to figure out that only one is needed, or whether it is defensive enough to accommodate cases where people use reflection to modify a final field, or whether it needs to be that defensive to accommodate binary compat (i.e. runtime version of baseclass NodeRef differs from compile time version).

@bbrehm bbrehm requested review from mpollmeier and ml86 February 20, 2023 12:14
Copy link
Contributor

@ml86 ml86 left a comment

Choose a reason for hiding this comment

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

Please remove the "wtflol" and add 1-2 sentence as explanation why we need different identifiers and than reference the bug issue. This makes it easier to understand.

@bbrehm bbrehm requested a review from ml86 February 20, 2023 12:46
Copy link
Contributor

@mpollmeier mpollmeier left a comment

Choose a reason for hiding this comment

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

wowsies

@mpollmeier mpollmeier self-requested a review February 20, 2023 13:06
Copy link
Contributor

@mpollmeier mpollmeier left a comment

Choose a reason for hiding this comment

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

@mpollmeier mpollmeier self-requested a review February 20, 2023 13:09
@mpollmeier
Copy link
Contributor

sorry for the approval/disapproval cycle, i was testing some local changes and must have been confused

@mpollmeier
Copy link
Contributor

fwiw this bug was "ported" to scala3

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