Skip to content

Raise error while changing immutable objects #878

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

Merged
merged 5 commits into from
Aug 4, 2022

Conversation

namannimmo10
Copy link
Collaborator

@namannimmo10 namannimmo10 commented Aug 3, 2022

Fixes #820

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good!

Comment on lines 2591 to 2594
} else if (ASR::is_a<ASR::Character_t>(*type)) {
throw SemanticError("'str' object does not support item assignment", x.base.base.loc);
} else if (ASR::is_a<ASR::Tuple_t>(*type)) {
throw SemanticError("'tuple' object does not support item assignment", x.base.base.loc);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about instead of chaining this with if-else, we create a new function say is_immutable to check whether the type is immutable or not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What other types are immutable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the examples is here: #820 (comment). Separating them to a new function might be easy to maintain.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the examples is here: #820 (comment). Separating them to a new function might be easy to maintain.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What other types are immutable?

3 of those which I am aware of, Tuple, Character, Frozenset.

is_immutable to check whether the type is immutable or not?

I agree here. Just doing the check in is_immutable function will also enhance readability, reusability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All python numeric types (numbers) are designed to be immutable. So I don't think having a separate function makes a lot of sense. Anyway, I still added it, so that it is readable for us.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well immutability is a very subjective (with respect to the language under consideration and the type under consideration) concept. In CPython, AFAIK, all objects are passed by reference or while assigning, a name (variable) points to an object. As you said all CPython numeric types are immutable. But that doesn't mean you can't change the value of a floating point name or an integer name (variable). You can definitely do that. However, when you assign one integer variable to another integer variable then two names (variables) start pointing to a common integer object. Changing one integer variable to another integer object doesn't make any modification to the original object (see example below).

>>> a = 12
>>> b = a
>>> id(a), id(b)
(4314587736, 4314587736) # 12 is an integer object with a, b names pointing to it
>>> b = 13
>>> id(a), id(b)
(4314587736, 4314587768) # a still points to 12, b points to 13

So immutability with respect to numeric types means you cannot modify 12 or 13 (both are integer objects in CPython, integer != integer objects (see https://docs.python.org/3/c-api/long.html). And anyways you cannot modify, 12, 13 or any constant number in LPython as well. You can modify an integer variable (name) though which is inline with what CPython does.

Now coming on to data types like tuple. They are containers and you can index them in an attempt to modify any single value present inside these containers. CPython doesn't allow that. But indexing and modifying a list object is allowed.

Bottomline - Immutability of constant numeric types is anyways retained implicitly. For enforcing immutability of container data types like tuple, string, frozenset, we need to en-force via checks as the one you are adding in this PR. In fact a user can make their class type sort of immutable by raising an error whenever __setitem__ is called. So having is_immutable will help in encapsulating tricky and complicated checks if needed in future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The added complication is that LPython restricts Python to such a subset so that we can modify the semantics to actually always copy things over for a = b, no references or reference counted pointers.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. In LPython, deepcopy happens whenever assigning one variable to another. I think that's the best way to not restrict performance. Like as we discussed with lists, we do deepcopy always when a = b (a and b both are lists).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@namannimmo10 namannimmo10 enabled auto-merge August 4, 2022 07:03
@namannimmo10 namannimmo10 merged commit 84df409 into lcompilers:main Aug 4, 2022
@namannimmo10 namannimmo10 deleted the str_assign branch August 4, 2022 07:21
Comment on lines +813 to +815
return ((ASR::is_a<ASR::Character_t>(*type) || ASR::is_a<ASR::Tuple_t>(*type)
|| ASR::is_a<ASR::Complex_t>(*type)));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@namannimmo10 This will fail for:

def ff():
    a: c32[2]
    a[0] = complex(1, 2)
    print(a)
    a[1] = complex(3, 4)
    print(a)

ff()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the error should only be raised for,

a.real = 1.0 or a.img = 2.0 (i.e., only for updating attributes of complex type). We can always assign a complex variable name to another value. The original value shouldn't have any change.

If there's an error can you fix this @Smit-create ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise error while changing immutable objects
4 participants