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
Fix compiler crash on xml attribute access on non xml:Element
values
#42598
base: master
Are you sure you want to change the base?
Conversation
4ae3140
to
58b2967
Compare
This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the |
@@ -8619,6 +8619,9 @@ private BType getLaxFieldAccessType(BType exprType) { | |||
return symTable.jsonType; | |||
case TypeTags.XML: | |||
case TypeTags.XML_ELEMENT: | |||
case TypeTags.XML_COMMENT: |
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.
My concern is why we include XML within the getLaxFieldAccessType
function. If the type is lax, then the lax field access rules are included. XML is not a lax type, correct?
@@ -314,7 +314,7 @@ public BType checkType(Location pos, | |||
public boolean isLaxFieldAccessAllowed(BType type) { | |||
type = Types.getImpliedType(type); | |||
Set<BType> visited = new HashSet<>(); | |||
return isLaxType(type, visited) == 1 || type.tag == TypeTags.XML || type.tag == TypeTags.XML_ELEMENT; | |||
return isLaxType(type, visited) == 1 || isAssignable(type, symTable.xmlType); |
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.
In the previous implementation, this was also incorrect. Lax field access should only be allowed for lax types. However, XML is not a lax type. I have concerns about including this logic within this function.
Purpose
Fixes #35191
Approach
Fix xml attribute access on all subtypes of xml
Samples
Remarks
Check List