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

Type names and union tags can't have the same name in Python target #68

Closed
dahlia opened this Issue Aug 24, 2016 · 12 comments

Comments

Projects
None yet
3 participants
@dahlia
Member

dahlia commented Aug 24, 2016

In Python target, because union tags are compiled to subtypes, if there's a name shared between a type and a union tag, one of them is overridden by other one. There can be also similar situation when there are tags of the same name but belonging to other unions.

Reported by @yjroot.

@dahlia dahlia added the typ:bug label Aug 24, 2016

@Kroisse

This comment has been minimized.

Contributor

Kroisse commented Aug 24, 2016

There is a several options:

Just restrict to use same name for type and union tag at the schema level.

  • Pros: backward-compatible; no need to fix the runtime.
  • Cons:
    • can broke schema previously written.
    • uncomfotable to define a union with tags for other records, like:
record coin ( ... );
record cash ( ... );
union payment = coin ( coin coin ) | cash ( cash cash );

For Python target, compile union tags to be the inner class of its union type like:

class Payment:
    # ...
    class Coin(Payment):
        # ...
    class Cash(Payment):
        # ...
  • Pros:
    • no need to change the schema
    • prevent namespace pollution by an union with numerous tags
  • Cons:
    • backward-incompatible with previously built packages
    • can be uncompotable in usual case
@dahlia

This comment has been minimized.

Member

dahlia commented Aug 24, 2016

@Kroisse I personally prefer the latter, but I think it can be mixed some pros of both. For example, if there aren't types of the same name, we can make a module-level alias for tags e.g. Coin = Payment.Coin. If there is already a type of the same name, we can do not define an alias for the tag with warning e.g. “A record type coin is already defined; alias for payment.coin will be not made.”

@admire93 Thoughts?

@Kroisse

This comment has been minimized.

Contributor

Kroisse commented Aug 24, 2016

@dahlia It could be hard to expect. Because both of union tag and record are compiled to the Python class, they can be mixed in the user's code easliy, and lead to a bug which is hard to discover what is wrong.

The user also can make an alias, so I think it's better to let them do. We don't need to give a bad default.

@dahlia

This comment has been minimized.

Member

dahlia commented Aug 24, 2016

The user also can make an alias, so I think it's better to let them do. We don't need to give a bad default.

So you're suggesting ways like using annotation, right?

@Kroisse

This comment has been minimized.

Contributor

Kroisse commented Aug 24, 2016

I'm prefer to provide an alias module, e.g.

from the_service import Payment
from the_service.payment import *

c = Coin( ... )  # from `the_service.payment.Coin`, alias of `Payment.Coin`
assert isinstance(c, Payment)

Still nasty, though.

@dahlia

This comment has been minimized.

Member

dahlia commented Aug 24, 2016

@Kroisse Unless I misunderstood it you are suggesting making a Python module named after the union name. For example, if there are payment union with coin/cash tags in the-service/entity.nrm the compile makes the following files, right?

  • the_service/__init__.py
  • the_service/entity/__init__.py
  • the_service/entity/payment.py
@kanghyojun

This comment has been minimized.

Member

kanghyojun commented Aug 24, 2016

I prefer the latter also. however i don't have a strong opinion about aliasing.

@Kroisse

This comment has been minimized.

Contributor

Kroisse commented Aug 24, 2016

And we need to discuss about the case that is the the origin of this issue. With this discussion, we can solve only the namespace conflict between union tags and other types. But some problem is still remained when I want to define several records and use them directly as a union:

from the_service import Payment, Coin

# too much "coin"
coin = Payment.Coin(coin=Coin(amount=100))
if isinstance(coin, Payment.Coin):
    print(coin.coin.amount)

Serialization is also problematic:

// serialization example of 'coin':
{
  "_tag": "coin",
  "coin": {
    "amount": 100
  }
}

updated: fixed "_type" to "_tag"

@Kroisse

This comment has been minimized.

Contributor

Kroisse commented Aug 24, 2016

@dahlia: That's right, and I understand that can be lead a circular import.

@dahlia

This comment has been minimized.

Member

dahlia commented Aug 24, 2016

In the case of "_type" field in the JSON payload we can ignore it for the reasons:

  1. "_type" doesn't affect to programs. It's only for humans.
  2. Union tags are encoded in "_tag" (which affects to programs) e.g. {"_type": "payment", "_tag": "coin", "coin": {"_type": "coin", "amount": 100}}.
@dahlia

This comment has been minimized.

Member

dahlia commented Aug 24, 2016

Instead of making an alias module (which may introduce another name conflicts), how about making Python target to provide aliasing option through annotation? E.g.:

union payment
    = @python-alias ("Coin")
      coin (coin coin)
    | @python-alias ("Cash")
      cash (cash cash)
    ;
@dahlia

This comment has been minimized.

Member

dahlia commented Apr 26, 2018

Since Nirum 0.4 the compile will become to disallow to use the same name for a type name and a union tag name (or an enum member name). See also PR #254.

@dahlia dahlia closed this Apr 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment