Skip to content
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 invalid subtyping relationship in xml #42221

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

poorna2152
Copy link
Contributor

Purpose

Fixes #34779

Approach

Update isAssignable to not to allow assigning of value with static typexml to value with static type xml:Comment|xml:Element|xml:ProcessingInstruction|xml:Text. But this assignment is allowed in the type binding pattern in theforeach loops. Thus in order to handle this case new function is introduced constituentTypesAssignable.

Samples

Provide high-level details about the samples related to this feature.

Remarks

List any other known issues, related PRs, TODO items, or any other notes related to the PR.

Check List

  • Read the Contributing Guide
  • Updated Change Log
  • Checked Tooling Support (#)
  • Added necessary tests
    • Unit Tests
    • Spec Conformance Tests
    • Integration Tests
    • Ballerina By Example Tests
  • Increased Test Coverage
  • Added necessary documentation
    • API documentation
    • Module documentation in Module.md files
    • Ballerina By Examples

@poorna2152 poorna2152 added the Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. label Feb 26, 2024
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.48%. Comparing base (e8b5bc5) to head (04cb468).
Report is 248 commits behind head on master.

Files with missing lines Patch % Lines
...llerinalang/compiler/semantics/analyzer/Types.java 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #42221      +/-   ##
============================================
- Coverage     77.55%   77.48%   -0.08%     
+ Complexity    58752    58448     -304     
============================================
  Files          3448     3432      -16     
  Lines        219728   218416    -1312     
  Branches      28922    28870      -52     
============================================
- Hits         170411   169234    -1177     
+ Misses        39921    39804     -117     
+ Partials       9396     9378      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

if (types.isAssignable(varType, typeNodeType)) {
if (types.constituentTypesAssignable(varType, typeNodeType)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we changing these? isAssignable should produce correct results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a special case. With the modification to isAssignable function in this PR, variable with static type of xml is not assignable to xml<xml:Element>|xml<xml:Comment>|xml<xml:ProcessingInstruction>|xml:Text or xml:Element|xml:Comment|xml:ProcessingInstruction|xml:Text(Which is the correct behaviour and which is expected).

The function given here is used to validate the types in a foreach loop. In a foreach loop context, variable with static type xml is assignable to loop variable of type xml<xml:Element>|xml<xml:Comment>|xml<xml:ProcessingInstruction>|xml:Text. Thus here it is not possible to use isAssignable

Example code,

function foo() {
    xml x = xml `<a></a><!--comment-->text<?data?>`;
    foreach xml:Element|xml:Comment|xml:ProcessingInstruction|xml:Text item in x {
        
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

The function given here is used to validate the types in a foreach loop. In a foreach loop context, variable with static type xml is assignable to loop variable of type xml<xml:Element>|xml<xml:Comment>|xml<xml:ProcessingInstruction>|xml:Text. Thus here it is not possible to use isAssignable

This definition isn't entirely correct, is it? In the loop/foreach case we should be checking the sequence member type (xml:Element|xml:Comment|xml:ProcessingInstruction|xml:Text) against the foreach variable type, not xml.

Copy link
Contributor Author

@poorna2152 poorna2152 Feb 29, 2024

Choose a reason for hiding this comment

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

Right, yes. But we cannot isAssignable for this check right, since for variable with static type xml it can't be assigned to xml:Element|xml:Comment|xml:ProcessingInstruction|xml:Text.

Copy link
Member

Choose a reason for hiding this comment

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

What I meant was that we should be using isAssignable with

  • source type - xml:Element|xml:Comment|xml:ProcessingInstruction|xml:Text against
  • target type - xml<xml:Element>|xml<xml:Comment>|xml<xml:ProcessingInstruction>|xml:Text

which will (should) evaluate to true.

xml is xml<xml:Element|xml:Comment|xml:ProcessingInstruction|xml:Text>, therefore, iteration value type is xml:Element|xml:Comment|xml:ProcessingInstruction|xml:Text. https://ballerina.io/spec/lang/master/#section_5.1.6

Copy link
Contributor Author

@poorna2152 poorna2152 May 17, 2024

Choose a reason for hiding this comment

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

In the case of type parameter the assignability is checked using the isAssignable(sourceParam, targetParam)
In the case of xml we cannot use this check.
Consider the following code

xml x = xml `<a>a</a>`;
x.forEach(function (xml entry) {
});

Here the targetType is xml:Element|xml:Comment|xml:ProcessingInstruction|xml:Text and the source type is xml. Since an xml value cannot be assigned to ``xml:Element|xml:Comment|xml:ProcessingInstruction|xml:Text` the check for xml is made special case for type params in this commit

Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

Copy link

github-actions bot commented Apr 3, 2024

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Apr 3, 2024
@poorna2152 poorna2152 removed the Stale label Apr 4, 2024
@lochana-chathura lochana-chathura self-requested a review April 9, 2024 14:55
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

Copy link

github-actions bot commented Jun 6, 2024

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Jun 6, 2024
@poorna2152 poorna2152 removed the Stale label Jun 7, 2024
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Jun 27, 2024
@poorna2152 poorna2152 removed the Stale label Jun 28, 2024
@poorna2152 poorna2152 force-pushed the xml_subtype_2 branch 2 times, most recently from 7182d77 to fefd6ed Compare July 8, 2024 03:52
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Jul 23, 2024
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added Stale and removed Stale labels Aug 30, 2024
@poorna2152 poorna2152 force-pushed the xml_subtype_2 branch 2 times, most recently from 621db18 to 310444d Compare September 5, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid subtype relation with xml
4 participants