Skip to content

Error on assignment to function parameter #1803

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 7 commits into from
May 18, 2023

Conversation

Shaikh-Ubaid
Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid commented May 13, 2023

towards #1784.

Comment on lines 201 to 214
if n < 0:
n = -n
n_: i32 = n
if n_ < 0:
n_ = -n_
prep = '-0b'
res: str
res = ''
if (n - _lpython_floordiv(n, 2)*2) == 0:
if (n_ - _lpython_floordiv(n_, 2)*2) == 0:
res += '0'
else:
res += '1'
while n > 1:
n = _lpython_floordiv(n, 2)
if (n - _lpython_floordiv(n, 2)*2) == 0:
while n_ > 1:
n_ = _lpython_floordiv(n_, 2)
if (n_ - _lpython_floordiv(n_, 2)*2) == 0:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems that the runtime library assigns to function parameters at several places. In this PR, I updated three such places. With respect to #1784 (comment), I would like to know if we should update all places in the runtime library to not allow assignment to function parameters or if we should keep allowing assignment to function parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not allow assigning to function parameters, unless the parameter is marked Out or InOut, but that's only allowed for arrays. Integers in Python can only be In, and we should not allow assigning to them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, got it.

@certik
Copy link
Contributor

certik commented May 13, 2023

Awesome, thanks for this!

Comment on lines 4626 to 4853
if (v->m_intent == ASR::intentType::In) {
throw SemanticError("Assignment to function parameter `" + std::string(v->m_name) + "` is not allowed", target->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.

Well there is one case like,

def f(x: ClassA) -> i32:
    x.Member1 = 2
    return x.Member1 * x.Member1

Should the above error or work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think/guess it should work. I think it is one of the ways to pass variables/values by reference in python. Objects are passed by reference in python. In the above example, we pass the object x and inside the function, a data member of x is updated. After the call to the function f, the changes to data members of x would be visible to the caller function.

For example, consider the following:

$ cat examples/expr2.py   
from lpython import i32, dataclass

@dataclass
class N:
    val: i32

def f1(n: i32):
    n = 2
    return

def f2(n: N):
    n.val = 2
    return

def main0():
    a: i32
    b: N = N(0)
    a = 4
    b.val = 4

    f1(a)
    f2(b)

    print(a)
    print(b.val)

main0()
$ python examples/expr2.py 
4
2
$ lpython examples/expr2.py
2
2

Copy link
Contributor

Choose a reason for hiding this comment

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

def f(x: ClassA) -> i32:
    x.Member1 = 2
    return x.Member1 * x.Member1

We have to decide what the default passing is, whether: In, InOut, Out. I would start with In, so the above is not allowed. To allow it, do:

def f(x: InOut[ClassA]) -> i32:
    x.Member1 = 2
    return x.Member1 * x.Member1

Then it is allowed.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #1807.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the following example:

def f(x: In[ClassA]) -> i32:
    x.Member1 = 2 # Assignment to Member1 which is a local variable of `x`
    x = ClassA(Member1=5, ...) # Assignment to x which is an intent In variable
    return x.Member1 * x.Member1

Do we print errors for both the above assignments or just for the second assignment (whose target variable has intent as In)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If in case we print error for the first assignment x.Member1 = 2 # Assignment to Member1 which is a local variable of x, do we need to print error in CPython as well? (I think/guess by default cpython allows assignment to class member variables).

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we print errors for both the above assignments or just for the second assignment (whose target variable has intent as In)?

I would print error for both. "In" means that "both the pointer and what it points to is constant". If you want to assign to any of it, you have to mark it "InOut".

Regarding CPython, I would not worry about it. The contract is: if LPython compiles it, it will run in CPython also. So if LPython enforces no modification, then CPython doesn't need to enforce it. In CPython we simply ignore type annotations.

@Shaikh-Ubaid
Copy link
Collaborator Author

For integration_tests/generics_02.py test case,

def swap(x: T, y: T):
temp: T
temp = x
x = y
y = temp
print(x)
print(y)
swap(1,2)

Should it work or fail?

@certik
Copy link
Contributor

certik commented May 13, 2023

This should fail, to allow it, do:

 def swap(x: InOut[T], y: InOut[T]): 
     temp: T 
     temp = x 
     x = y 
     y = temp 
     print(x) 
     print(y) 

Also note that Out and InOut in Python only really works for non-simple types, so not for ints or reals. See #1807.

@Shaikh-Ubaid Shaikh-Ubaid force-pushed the input_param_redef branch 2 times, most recently from 6e177f1 to ab93ae7 Compare May 13, 2023 16:01
@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review May 13, 2023 16:01
@Shaikh-Ubaid
Copy link
Collaborator Author

Ready.

@Shaikh-Ubaid Shaikh-Ubaid requested a review from certik May 13, 2023 16:03
@@ -509,7 +509,7 @@ RUN(NAME test_pkg_lnn_01 LABELS cpython llvm)
RUN(NAME test_pkg_lnn_02 LABELS cpython llvm)

RUN(NAME generics_01 LABELS cpython llvm c)
RUN(NAME generics_02 LABELS cpython llvm c)
# RUN(NAME generics_02 LABELS cpython llvm c)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I temporarily commented this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I made the PR a draft until we resolve it. First let's get everything else working, then we'll fix this.

@certik certik marked this pull request as draft May 13, 2023 16:14
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.

I think that this is good.

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review May 18, 2023 04:22
@Shaikh-Ubaid
Copy link
Collaborator Author

This PR prints errors on assignments to simple variables with intent In. I will handle other cases like assignments to struct local variables where the struct is passed with intent In in a new PR.

@Shaikh-Ubaid Shaikh-Ubaid enabled auto-merge May 18, 2023 04:29
@Shaikh-Ubaid Shaikh-Ubaid merged commit 3665bcf into lcompilers:main May 18, 2023
@Shaikh-Ubaid Shaikh-Ubaid deleted the input_param_redef branch May 18, 2023 11:22
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.

3 participants