modularml / mojo

The Mojo Programming Language
https://docs.modular.com/mojo/manual/
Other
22.88k stars 2.58k forks source link

[BUG]: Trailing comma is not supported in assignemnts #1649

Open kishmakov opened 7 months ago

kishmakov commented 7 months ago

Bug description

Python supports trailing comma for multiple targets in assignment expressions, while Mojo doesn't. It contradicts the goal to become a Python superset.

Steps to reproduce

Here is a snippet which demonstrates the issue.

def main():
    var a: Int = 0
    var b: Int = 0
    a, b = 1, 2 # works
    a, b, = 1, 2 # fails
    print("a is", a)
    print("b is", b)

System information

$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 22.04.2 LTS
Release:    22.04
Codename:   jammy
$ mojo --version
mojo 0.6.3 (f58f8b94)
$ modular -v
modular 0.2.2 (95d42445)
gryznar commented 7 months ago

I am against supporting trailing comma. It leads to strange bugs if sth like this would be used:

x = 1,  # comma is accidental and thus type of x is tuple[int] instead of int

IMHO Mojo should not support Python weird quirks which more often lead to strange bugs than to increased usability. Mojo cannot be compiled Python and sacrificing compatibility in such cases seems to be a step in good direction

kishmakov commented 7 months ago

By the way, this issue (x = 1,) looks rather like a tooling issue to me, not like a language issue.

With proper tooling support, such accidental bugs can be marked with diagnostic hints.

gryznar commented 7 months ago

So, why Mojo would have to support this controversial syntax if tooling would highlight that?

kishmakov commented 7 months ago

@gryznar I think you better address this question either to Modular team (superset approach) or to Python team (initial language design).

Every choice obviously has its pros and cons. And there is some motivation behind it.

gryznar commented 7 months ago

@kishmakov Yeah, my all remarks are addressed directly to Modular team to not introduce this "missing feature"

Not all Python behavior is possible to replicate in compiled language, so, not all features can be applicated directly. Mojo cannot be strict superset, so it has to compromise. If so, compromising here to avoid strange bugs seems like not a bug and that is what I would to say. Bare comma at the end, without parentheses is not what I ever have to use in Python, same as most users, so really, this looks for me like a good decision, that Mojo does not support that

Personally, Mojo will be the best if it would be a Python++ and not emulated Python. I agree that all these incompatible decision should be published in FAQ with answer to "Why ... is not supported?"

laszlokindrat commented 7 months ago

Thanks for opening this ticket! We discussed the problem of trailing commas in unpacking as well as in function call expressions (e.g. foo(a, b,), which is supported in Python). We decided that this is something we want to support; perhaps it looks quirky, but it is part of Python, and syntactic compatibility with Python is an important goal of Mojo. We acknowledge that in some cases syntactic compatibility might be at odds with building a better Python, but we think this particular feature poses no such risks. This means, that we will treat this ticket as a genuine bug, and will try to fix it.

gryznar commented 7 months ago

But, the question is: what is the added value from this support? As I stated it is not useful in most cases and may lead to very strange errors. I eencountered such one and it was waste of time to track this because Python automatically converted trailing comma to unwanted tuple. Really, this is the latest feature which I would Mojo to support. I propose to make a pool to community for such quirks and ask if it is desired or not.

You can always add it, but it's harder to remove later.

soraros commented 7 months ago

@laszlokindrat Can we at least have the LSP generating warnings/fixes like "redundant trailing comma" etc.? Also, will code compatibility be that much of a problem? Mechanical upgrader is needed (wrapping certain names in backquotes, e.g.) any ways.

gryznar commented 7 months ago

@laszlokindrat as you can see, there are voices from community which do not want this feature. Please, do not ignore them. Pylint treats trailing comma like a possible bug: https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/trailing-comma-tuple.html. If there are hundreds of bugs in Mojo to fix, do we really, really want to focus on adding support for such buggy feature? If linters highlight that as possible bug, why to support it at all?

laszlokindrat commented 7 months ago

@gryznar @soraros I agree that there are issues which are much more pressing than this. Our decision above was with respect to what we would like to achieve in the fullness of time: syntactic compatibility with Python. Notably, we did not decide to prioritize this: it might happen soon if some other related feature gets prioritized and the fix becomes easy, or it might be a while before we have a fix.

But, the question is: what is the added value from this support?

One of the reasons why trailing commas are something we need to support is they provide a way to control formatting in a Pythonic way. For example, adding a trailing comma will trigger black to separate the expressions to multiple lines, and potentially add a pair of parentheses as well, e.g. a, b, = 1, 2, becomes

(
    a,
    b,
) = (
    1,
    2,
)

Python users do rely on this feature, and it is reasonable to assume that Mojo users would too.

Pylint treats trailing comma like a possible bug

This seems like a reasonable thing to do in a linter, where users can also opt out of it. But that doesn't mean that language should disallow it.

gryznar commented 7 months ago

I am still not convinced. How about opening a discussion / poll and asking community itself? Maybe there is no such demand at all?

gryznar commented 7 months ago

@laszlokindrat please at least reject tuple creation or more generally - signature change using trailing comma. This will prevent accidental mistakes like:

x = 1, #  type of rhs is tuple instead of int

The rest of usecases related to formatting only which does not impact type or signature, may be indeed usable

laszlokindrat commented 7 months ago

@gryznar Of course we would love to hear more from the community! As you pointed out though, this issue is not super high priority so, at this time, I don't see a point driving a bigger discussion around this. Feel free to leave comments on this ticket; we are following all these threads and we are keeping an open mind.

LiamSwayne commented 6 months ago

I think the trailing comma and other syntactic peculiarities should be removed because Mojo already will not be a strict superset of Python. Python already occupies the ^ operator for a bit operation, and Mojo's use of it as a transfer directly conflicts with it's use in Python, so Mojo is not a strict superset of Python unless the transfer operator is removed or renamed, which seems highly unlikely. Since that means Mojo will be a loose superset, it seems reasonable for a loose superset to avoid Python's pitfalls.

LiamSwayne commented 6 months ago

I think it's also worth noting that ~, |, >>, << and & also could not be repurposed if Mojo plans on being a strict superset. These are Python features that very few Python developers use, but occupy quite a few symbols that could have other uses.

soraros commented 6 months ago

@LiamSwayne The binary ^ in Mojo is still the same bit operation as in Python, and I don't see why it's in conflict; if using the same symbol for two things is desirable is a separate issue.

lattner commented 5 months ago

This is a simple parser omission. Adding it improves python compatibility and comes at no cost.