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

How to handle fields in subclasses when super classes have fields with same name? #42

Open
smarr opened this issue Jun 9, 2020 · 6 comments
Labels
bug Fixes an issue, incorrect implementation spec Needs specification, or is specification related.

Comments

@smarr
Copy link
Member

smarr commented Jun 9, 2020

As reported by @Hirevo, how do we want to handle it when subclasses define fields with the same name as fields in their super classes?

TestA = ( | a | foo = ( a println. a := 10. a println. ))

TestB = TestA ( | a | 
  run = (
    a := 1.
    a println.
    super foo.
     a println.
  )
)

This currently seems to ignore the field defined in TestB.
In yksom (@ltratt) it seems to crash according to @Hirevo.

A sensible language would probably shadow field a from TestA, which means it can't be accessed from TestB, and only TestB.a can be accessed as a separate field.

@smarr smarr added bug Fixes an issue, incorrect implementation spec Needs specification, or is specification related. labels Jun 9, 2020
@Hirevo
Copy link

Hirevo commented Jun 9, 2020

Another question related to this one is:
What is the interaction of such shadowing with the Class>>#fields primitive ?

Considering those same TestA and TestB, consider the following additional class:

TestC = (
    formatList = (       "just some fancy list formatting magic"
        | count |
        count := 0.
        ^ [ :field |
            (count > 0) ifTrue: [ ' ' print ].
            count := count + 1.
            field print
        ]
    )

    run = (
        'TestA fields: #(' print.
        TestA fields do: self formatList.
        ')' println.
        'TestB fields: #(' print.
        TestB fields do: self formatList.
        ')' println.
    )
)

som-java, CSOM and som-rs all output the following when running TestC:

TestA fields: #(#a)
TestB fields: #(#a #a)

While yksom outputs the following:

TestA fields: #(#a)
TestB fields: #(#a)

So, who is right here ?
If shadowing is the expected behavior, then the first output would be the correct one, if I am not mistaken ?

@smarr
Copy link
Member Author

smarr commented Jun 9, 2020

Hm, another good question.
I would think the most useful version is to only list the fields defined by the current class, and expect the user to go to the superclass for other relevant fields.

@ltratt
Copy link

ltratt commented Jun 9, 2020

This is a good test case -- well done @Hirevo!

I would strongly recommend not trying to be clever and trying to allow subclasses to have their own fields of the same name. In a dynamically typed language this opens up a world of pain for very little gain (in a statically typed language it's also a pain, but slightly less so). My suggestion is simply to forbid subclasses from redefining a name that's used in a superclass. [The subclass can, of course, still access the appropriate field, but it can't pretend to redefine it.] This also has the pleasing bonus of being simple to implement!

@ltratt
Copy link

ltratt commented Jun 9, 2020

My suggestion is simply to forbid subclasses from redefining a name that's used in a superclass.

I've prototyped this in softdevteam/yksom#147 (and detected another related case which I think is worth explicitly handling). Thoughts welcome!

@smarr
Copy link
Member Author

smarr commented Jun 9, 2020

Just for context: The historical SOM had independent class loading. With this I mean that classes could be parsed without the super class having to be present.
Pretty much* as modular compilation in Java works.

This means, it wasn't possible to know the fields of the superclass.
As in Java, the offsets for accesses had to be resolved at run time.

Current SOM requires the super class to be loaded before hand, which leads to a possible recursion in the parser. Don't think that the parser was reentrant originally.
Anyway, I think I made SOM more "static" than it used to be.

In the interest of simplicity, the extra static checks are probably a good tradeoff.

@ltratt
Copy link

ltratt commented Jun 9, 2020

In the interest of simplicity, the extra static checks are probably a good tradeoff.

Cool. I'll ask for that to be reviewed for inclusion into yksom.

bors bot added a commit to softdevteam/yksom that referenced this issue Jun 10, 2020
147: Detect overlapping field names r=vext01 a=ltratt

There are two cases to be dealt with:

1. A field name has already been defined in a superclass (5e00d91).
2. A class defines the same field more than once (9370609).

The first of those was pointed out by @Hirevo in SOM-st/SOM#42; the second is a relatively simple variation on the same theme.

Co-authored-by: Laurence Tratt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes an issue, incorrect implementation spec Needs specification, or is specification related.
Projects
None yet
Development

No branches or pull requests

3 participants