bmx-ng / bcc

A next-generation bcc parser for BlitzMax
zlib License
33 stars 13 forks source link

Bug with extending generic types #639

Closed thareh closed 6 months ago

thareh commented 7 months ago

Good evening,

Here's another bug I found related to generics (Sorry!):

utils.mod/nullable.mod

Module Utils.Nullable

Framework BRL.Blitz

Rem
bbdoc:
EndRem
Type TGNullable<T>

Private
    Field nil:Int = True
    Field val:T

Public
    Rem
    bbdoc:
    EndRem
    Method New(val:T)
        SetValue(val)
    EndMethod

    Rem
    bbdoc:
    EndRem
    Method IsNull:Int()
        Return nil
    EndMethod

    Rem
    bbdoc:
    EndRem
    Method SetNull()
        nil = True
    EndMethod

    Rem
    bbdoc:
    EndRem
    Method UnsetNull()
        nil = False
    EndMethod

    Rem
    bbdoc:
    EndRem
    Method SetValue(val:T, unset:Int=True)
        Self.val = val

        If unset
            UnsetNull()
        EndIf
    EndMethod

    Rem
    bbdoc:
    EndRem
    Method GetValue:T(safe:Int=True)
        If safe And nil
            Return Null
        Else
            Return val
        EndIf
    EndMethod

    Rem
    bbdoc:
    EndRem
    Method Differs:Int(val:TGNullable<T>)
        If nil <> val.nil
            Return True
        EndIf

        Return Self.val <> val.val
    EndMethod

    Method Operator=(val:TGNullable<T>)
        Return Not Differs(val)
    EndMethod

EndType
Framework BRL.Blitz
Import BRL.StandardIO
Import Utils.Nullable

Type TNullableInt Extends TGNullable<Int>
EndType

Local val:TNullableInt = New TNullableInt()
val.SetValue(10)
Print val.IsNull()
Print val.GetValue()

Works like a charm, but if I move the type TNullableInt to the module I get the error "Wrong number of type arguments for class TGNullable".

Thanks!

woollybah commented 6 months ago

I've pushed some changes to bcc that should allow this to work now. It's also sorted some other issues in related areas, but there's a reasonable chance I may have broken something else along the way...

Have a play and let me know how you get on.

thareh commented 6 months ago

Awesome! Thanks a bunch! Will have a go at it and get back to you.

thareh commented 6 months ago

Seems to work very well, no issues with other projects either - awesome!

I'd be cool to replace TList with TList Extends TLinkedList< Object > and TMap with TMap Extends TTreeMap<Object, Object> etc eventually so they all share the same codebase.

Cheers!

woollybah commented 6 months ago

Indeed, but it'll only work properly with a version of bcc that handles it and produces the correct code.

I actually want to split out the "extra" maps into their own module so I can convert them to use generics, so TStringMap would be a TTreeMap<String,Object>, etc.

I'm undecided. Maybe we should have a collections module, and work from there...

GWRon commented 6 months ago

Having a way to "test" the TStringMap, TIntMap, TPtrMap, ... this way sounds good. I am using some of them more or less heavily - so "discrepancies" or issues during compilation might be easier to spot.

Maybe we should have a collections module, and work from there...

I suggest to store them in brl ... maybe "brl.mod/collections.mod" ? :D

Or are you talking about something like the "math.mod" - so some new "main scope" thing (mods/collections.mod/map.mod) ?

woollybah commented 6 months ago

maybe "brl.mod/collections.mod"

The problem putting "instances" of everything in there, is, you will get each of those in your final binary. You may not want 6 different TMap-type maps compiled into your app.

Anyway, you may not even want a TStringMap, unless you are really putting random Object values into it. Instead, you could have a TTreeMap<String, TMyClass> for a given type, which should be slightly more efficient, in theory - well, it'll certainly be more strongly typed than looking at everything as "Object".

HurryStarfish commented 6 months ago

Personally I would love to see (or write) a proper, generic new collections module, but that of course needs fully functioning generics first. And then, for it to be well-designed, also a couple of other features that we don't have yet. I think the old TList/TMap should definitely be left alone, not become aliases of new collection types. I don't see much value in that and changes to the new module could easily break existing old code or the API. The same goes for TStringMap etc.; once there are a fully working generic collections, those types would then all be obsolete and only need to be kept around for legacy code. New code should then always use the new e.g. TTreeMap<K, V> or THashMap<K, V> (implementing a common IMap<K, V> etc.)

HurryStarfish commented 6 months ago

The problem putting "instances" of everything in there, is, you will get each of those in your final binary. You may not want 6 different TMap-type maps compiled into your app.

Generics will invariably have that problem during code generation, if implemented like C++ templates (or in Rust, I think). Generating a separate concrete type for every combination of type arguments makes binaries large. There are alternatives, but they're harder to implement in the compiler and have their own trade-offs.

GWRon commented 6 months ago

The problem putting "instances" of everything in there, is, you will get each of those in your final binary. You may not want 6 different TMap-type maps compiled into your app.

I guess you will have that problem with any module being autoloaded (and you not using the framework command).

At the end it is a matter of "grouping things together" ... Maybe add them to a new module scope ..and bring in the "defaults" similar to brl.random (and random.mod-modulescope).

Offtopic: a pity we only have moduleparent.module scope depth...

import brl.collection.queue 'imports Queue but not Set, Bag...
Import brl.collection 'imports all
HurryStarfish commented 6 months ago

bring in the "defaults" similar to brl.random

I understand why this was done for the legacy APIs, but probably isn't a great approach for new stuff; interfaces are a better mechanism for that.

Offtopic: a pity we only have moduleparent.module scope depth...

Yeah, it would be nice to be able to have names with more components. I'm not so sure about this though:

Import brl.collection 'imports all

I wouldn't really expect an Import to automatically import every module with a "more nested" name. That could also cause issues, e.g.: adding a new module in your mod folder suddenly breaks code that you haven't changed. There could be different syntax for that, but then again maybe that's not necessary. (At least, it's not possible in C# either and I've never really needed it there, can always just import what's actually used.)