amazon-ion / ion-python

A Python implementation of Amazon Ion.
https://amazon-ion.github.io/ion-docs/
Apache License 2.0
253 stars 50 forks source link

Symbols lack SIds when C-extension is enabled #316

Open rmarrowstone opened 6 months ago

rmarrowstone commented 6 months ago

What I expect:

>>> from amazon.ion.simpleion import loads
>>> loads('$1')
IonPySymbol(text='$ion', sid=1, location=ImportLocation(name='$ion', position=1))
>>>
>>>
>>>

What happens:

>>> from amazon.ion.simpleion import loads
>>> loads('$1')
IonPySymbol(text='$ion', sid=None, location=None)
>>>
>>>
>>>

Confirming this is c-extension specific (continued from same repl session above):

>>> from amazon.ion import simpleion
>>> simpleion.c_ext
True
>>> simpleion.c_ext = False
>>> loads('$1')
IonPySymbol(text='$ion', sid=1, location=ImportLocation(name='$ion', position=1))
>>>
tgregg commented 6 months ago

All three variants are "correct", in that when the text is known, nothing else matters. Is this a concern about consistency across the reader options?

rmarrowstone commented 6 months ago

When the text is not known, the information about SId and import location is also missing.

I see. That currently raises an error as well, though we could be returning a SymbolToken with None text and Sid and import location.

Update: there is a correctness issue regarding imported Symbols with undefined text. As it is now, imported symbols with undefined text are considered equivalent to each other (regardless of SId) and local symbols with undefined text.