-
Notifications
You must be signed in to change notification settings - Fork 165
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
Added static methods for classes #2721
Conversation
The design would be the following: class test:
def fn():
pass to StructType test {
class procedure :: fn => f
def f:
pass
} |
Here is the simple example from LFortran for member function: module m
type t
integer :: x
integer :: y
contains
procedure :: fn => f
end type t
contains
integer function f(this)
class(t), intent(in) :: this
end function
end module
program test
use m
implicit none
type(t) :: t1
print *, t1%fn()
end program test |
Let's mark it as draft PR until it is ready to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation seems good, there are minor improvements. We will do it once the MRE works.
Also, for the code generation error, we need to visit StructType in the Module scope in LLVM backend (asr_to_llvm.cpp).
In LFortran the Function is available in the Module scope but here we keep it in the StructType scope, right? So, visit the symbols of StructType in the Module visitor.
} else if (AST::is_a<AST::Pass_t>(*x.m_body[i])) { | ||
continue; | ||
} else if (!AST::is_a<AST::AnnAssign_t>(*x.m_body[i])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this changes, it seems very confusing, we will it in another PR
diff --git a/src/lpython/semantics/python_ast_to_asr.cpp b/src/lpython/semantic
s/python_ast_to_asr.cpp
index 01031d704..8c9dd2dee 100644
--- a/src/lpython/semantics/python_ast_to_asr.cpp
+++ b/src/lpython/semantics/python_ast_to_asr.cpp
@@ -3194,12 +3194,14 @@ public:
x.base.base.loc);
}
SymbolTable *parent_scope = current_scope;
- ASR::symbol_t* clss_sym = current_scope->get_symbol(x_m_name);
+ ;
ASR::StructType_t* clss = nullptr;
- if(clss_sym != nullptr && !is_enum(x.m_bases, x.n_bases) && !is_union(
x.m_bases, x.n_bases)){
- clss = ASR::down_cast<ASR::StructType_t>(clss_sym);
- current_scope = clss->m_symtab;
- is_generating_body = true;
+ if(ASR::symbol_t* clss_sym = current_scope->get_symbol(x_m_name) ) {
+ if ( is_a<ASR::StructType_t>(*clss_sym) ){
+ clss = ASR::down_cast<ASR::StructType_t>(clss_sym);
+ current_scope = clss->m_symtab;
+ is_generating_body = true;
+ } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test and make other tests to pass.
Let's finish off this and move on to |
fixes #2722 |
@@ -1,9 +1,5 @@ | |||
semantic error: Struct member functions are not supported | |||
--> tests/errors/structs_10.py:7:5 - 8:24 | |||
semantic error: The type 'StringIO' is undeclared. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for struct, we shouldn't allow member functions.
@certik, What do you think?
@@ -0,0 +1,116 @@ | |||
(TranslationUnit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be removed.
class Test: | ||
mem : i64 = i64(5) | ||
s : str = "abc" | ||
def fn_1(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print the values or texts are not being test, so I suggest to pass some arguments to the functions and check the computed return values.
assert t.s == "abc" | ||
Test.fn_1() | ||
|
||
main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: make sure to add a newline at the end of the file.
fn_2() | ||
print("fn_3 called") | ||
Test.fn_3() | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return | |
return |
No extra spaces as well
current_scope = current_scope_copy; | ||
} | ||
|
||
void make_struct_f_def(const ASR::StructType_t &x){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have time, try simplifying this function: visit_StructType and make_struct_f_def.
They seem duplicated
c_arg.loc = var_sym->base.loc; | ||
c_arg.m_value = init_expr; | ||
member_init.push_back(al, c_arg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems repeated as well.
I will try to look into them
Static method typically means class method --- it doesn't access any members of the instance. I think in this case you mean a method that is not virtual? How are class variables implemented in ASR --- can you show how the ASR looks like for the above class? |
Before we go too deep into this, I think we need to use Class for classes, not Struct. Otherwise it will be hard to undo. |
@@ -529,11 +529,13 @@ class VerifyVisitor : public BaseWalkVisitor<VerifyVisitor> | |||
ASR::is_a<ASR::StructType_t>(*a.second) || | |||
ASR::is_a<ASR::UnionType_t>(*a.second) || | |||
ASR::is_a<ASR::ExternalSymbol_t>(*a.second) || | |||
ASR::is_a<ASR::CustomOperator_t>(*a.second) ) { | |||
ASR::is_a<ASR::CustomOperator_t>(*a.second) || | |||
ASR::is_a<ASR::Function_t>(*a.second)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the issue here? Why is Function now skipped?
I forgot about the Class symbol and thought to represent ASR the same as LFortran. Sorry about that. ASR
(TranslationUnit
(SymbolTable
1
{
__main__:
(Module
(SymbolTable
2
{
Test:
(StructType
(SymbolTable
4
{
fn_1:
(Function
(SymbolTable
5
{
Test_fn_3:
(ExternalSymbol
5
Test_fn_3
4 fn_3
Test
[]
fn_3
Public
)
})
fn_1
(FunctionType
[]
()
Source
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
.false.
)
[fn_2]
[]
[(Print
[(StringConstant
"Inside fn_1"
(Character 1 11 ())
)]
()
()
)
(Print
[(StringConstant
"fn_2 called"
(Character 1 11 ())
)]
()
()
)
(SubroutineCall
2 fn_2
()
[]
()
)
(Print
[(StringConstant
"fn_3 called"
(Character 1 11 ())
)]
()
()
)
(SubroutineCall
5 Test_fn_3
()
[]
()
)
(Return)]
()
Public
.false.
.false.
()
),
fn_3:
(Function
(SymbolTable
6
{
})
fn_3
(FunctionType
[]
()
Source
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
.false.
)
[]
[]
[(Print
[(StringConstant
"Inside fn_3"
(Character 1 11 ())
)]
()
()
)]
()
Public
.false.
.false.
()
),
mem:
(Variable
4
mem
[]
Local
()
()
Default
(Integer 8)
()
Source
Public
Required
.false.
),
s:
(Variable
4
s
[]
Local
()
()
Default
(Character 1 -2 ())
()
Source
Public
Required
.false.
)
})
Test
[]
[mem
s]
Source
Public
.false.
.false.
[((Cast
(IntegerConstant 5 (Integer 4))
IntegerToInteger
(Integer 8)
(IntegerConstant 5 (Integer 8))
))
((StringConstant
"abc"
(Character 1 3 ())
))]
()
()
),
__main__global_stmts:
(Function
(SymbolTable
8
{
})
__main__global_stmts
(FunctionType
[]
()
Source
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
.false.
)
[main]
[]
[(SubroutineCall
2 main
()
[]
()
)]
()
Public
.false.
.false.
()
),
fn_2:
(Function
(SymbolTable
3
{
})
fn_2
(FunctionType
[]
()
Source
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
.false.
)
[]
[]
[(Print
[(StringConstant
"Inside fn_2"
(Character 1 11 ())
)]
()
()
)
(Return)]
()
Public
.false.
.false.
()
),
main:
(Function
(SymbolTable
7
{
Test_fn_1:
(ExternalSymbol
7
Test_fn_1
4 fn_1
Test
[]
fn_1
Public
),
t:
(Variable
7
t
[]
Local
()
()
Default
(Struct
2 Test
)
()
Source
Public
Required
.false.
)
})
main
(FunctionType
[]
()
Source
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
.false.
)
[]
[]
[(Assignment
(Var 7 t)
(StructTypeConstructor
2 Test
[((Cast
(IntegerConstant 5 (Integer 4))
IntegerToInteger
(Integer 8)
(IntegerConstant 5 (Integer 8))
))
((StringConstant
"abc"
(Character 1 3 ())
))]
(Struct
2 Test
)
()
)
()
)
(Print
[(StructInstanceMember
(Var 7 t)
4 mem
(Integer 8)
()
)]
()
()
)
(Assert
(IntegerCompare
(StructInstanceMember
(Var 7 t)
4 mem
(Integer 8)
()
)
Eq
(Cast
(IntegerConstant 5 (Integer 4))
IntegerToInteger
(Integer 8)
(IntegerConstant 5 (Integer 8))
)
(Logical 4)
()
)
()
)
(Print
[(StructInstanceMember
(Var 7 t)
4 s
(Character 1 -2 ())
()
)]
()
()
)
(Assert
(StringCompare
(StructInstanceMember
(Var 7 t)
4 s
(Character 1 -2 ())
()
)
Eq
(StringConstant
"abc"
(Character 1 3 ())
)
(Logical 4)
()
)
()
)
(SubroutineCall
7 Test_fn_1
()
[]
()
)]
()
Public
.false.
.false.
()
)
})
__main__
[]
.false.
.false.
),
main_program:
(Program
(SymbolTable
9
{
__main__global_stmts:
(ExternalSymbol
9
__main__global_stmts
2 __main__global_stmts
__main__
[]
__main__global_stmts
Public
)
})
main_program
[__main__]
[(SubroutineCall
9 __main__global_stmts
2 __main__global_stmts
[]
()
)]
)
})
[]
) |
@tanay-man, as @certik suggested instead of Use |
Currently due to 'self' not being implemented, we cannot access instance variables. I think static methods is correct. |
Also, @tanay-man, as we have to create ClassType, we need it handle everywhere, like in asr_verify, declaration, Constructor, etc. |
No description provided.