Error Message (Dict,Set,List ) Type Mismatch#2577
Error Message (Dict,Set,List ) Type Mismatch#2577assem2002 wants to merge 10 commits intolcompilers:mainfrom
Conversation
assem2002
changed the title
|
I Edited it, #Thanks @kmr-srbh |
assem2002
changed the title
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
I am unsure if this is a good approach. We already catch the SemanticError() in body_visitor().
There was a problem hiding this comment.
- I don't quite get what you mean by a catch in the same class, I would be glad to know what you have in mind so I can improve the code.
- I think you mean i have a way of accessing the error without using the try..catch.
- error recovery is a great idea to have in such a situation -as i read about-, but for now we just brute force on the cases we face; If you want to print both errors i could print out the semantic error and then the semantic abort.
There was a problem hiding this comment.
Thanks for this PR @assem2002. I appreciate your work. The changes in this PR look good but they seem to be more like hard coded for the specific test and might not be needed at the moment.
I think a better (and more accurate) way to resolve the issue #2042 is to implement error recovery mechanisms (which may/might be a larger issue to be dealt with at the moment).
|
Please mark this PR ready for review once it is ready. |
|
The problem we face in issue #2042 appears to be more complex. @Shaikh-Ubaid suggested an error recovery mechanism to be implemented in the system to handle such multiple errors while doing ASR construction. Probably we should make a bigger issue with specified tasks as long as there's no such mechanism or similar ones already implemented. I'll draft the PR,but I think the issue should be closed unless a hard-coded fix is fine for now. |
| ASR::ttype_t* list_type = ASRUtils::TYPE(ASR::make_List_t(al, x.base.base.loc, type)); | ||
| tmp = ASR::make_ListConstant_t(al, x.base.base.loc, list.p, | ||
| list.size(), list_type); | ||
| throw SemanticError("All List elements must be of the same type for now", |
There was a problem hiding this comment.
I have the following idea in mind, check if it's possible to implement or handle. If any blockers, do let me know.
Instead of throw, do diag.add(diag::Diagnostic(...)) and return. Check if diag has some error and throw error based on that in visit_Assign.
There was a problem hiding this comment.
Okay this is doable,but I need to know what benefit we'll get after replacing a throw statement with diag.add(),so I can reason my fix as I don't quite get what you mean .
I think I could handle the error directly without creating ASR element. If that what you imply, that would make the code change in the visit_assign() only, and (visit_set() , visit_dict() , visit_list() ) would remain the same.
assem2002
changed the title
|
The fix in this PR doesn't seem to be a high priority. I think we should fix it issue by adding support for error-recovery (as mentioned in #2042 (comment)), which in itself is a larger issue. Should we tackle this later @certik? |
|
Yes, correct. We have to extend the ASR->ASR transformation to just report all errors and recover from them. We should not be catching exceptions like that. In fact ideally we should not use exceptions at all. I think we should be able to recover on the level of statements, and then have special handling in blocks of statements (functions). In terms of priorities, first we should deliver beta quality ASR and LLVM (via LFortran) for compiling valid code. Only then focus on beautiful error messages for invalid code. |
5 participants
Fixes #2042
The Fix
Screen Shot Of The Code In The Issue Description After Fix.
The issue we're facing could be described as "CONSTRUCTION WHILE ASSIGNING" problem.
It happens when visit_assign function is invoked.
It could happen with annassign function also as shown below
Mention To Some Other Cases
The issue appears in the following code:
And also here:
**Let me know if the object construction handling is a concern and needs to be debugged in some other way.
Simplifying the code
We could write the type check code as a function under ASRUtiles namespace, as it's redundant piece of code here.
I don't have deep grasp on the visit_assign function so I didn't mess with its flow because - as I think - there's type coercion happening.
I just added a piece of code to solve the problem, but I think the redundancy of the code we face could be handled easily by doing early type check and remove the latter one.