Preserve the determined type of constant value expressions #38183

sanjana · 2022-10-12T12:03:49Z

Purpose

$title

Fixes #36831

Approach

Samples

Remarks

Check List

…lver

codecov · 2022-10-12T13:36:07Z

Codecov Report

Base: 76.64% // Head: 76.65% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (2e18924) compared to base (132861c).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #38183      +/-   ##
============================================
+ Coverage     76.64%   76.65%   +0.01%     
- Complexity    53050    53056       +6     
============================================
  Files          3354     3354              
  Lines        199180   199195      +15     
  Branches      25841    25842       +1     
============================================
+ Hits         152655   152693      +38     
+ Misses        37849    37839      -10     
+ Partials       8676     8663      -13

Impacted Files	Coverage Δ
...iler/semantics/analyzer/ConstantValueResolver.java	`84.61% <66.66%> (-0.12%)`	⬇️
...a/runtime/internal/scheduling/RuntimeRegistry.java	`62.26% <0.00%> (-20.76%)`	⬇️
...internal/repositories/RemotePackageRepository.java	`79.59% <0.00%> (-1.54%)`	⬇️
...a/io/ballerina/converters/JsonToRecordRequest.java	`54.54% <0.00%> (-1.02%)`	⬇️
...ballerinalang/central/client/CentralAPIClient.java	`47.92% <0.00%> (-0.40%)`	⬇️
...rinalang/compiler/semantics/model/SymbolTable.java	`96.65% <0.00%> (-0.15%)`	⬇️
...io/ballerina/converters/JsonToRecordConverter.java	`83.41% <0.00%> (-0.09%)`	⬇️
...io/ballerina/semver/checker/diff/NodeDiffImpl.java	`71.42% <0.00%> (ø)`
...erina/runtime/internal/BallerinaXmlSerializer.java	`81.10% <0.00%> (+0.08%)`	⬆️
...erina/converters/JsonToRecordConverterService.java	`95.83% <0.00%> (+0.18%)`	⬆️
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

gimantha · 2022-10-13T08:21:50Z

tests/ballerina-compiler-api-test/src/test/resources/test-src/expressions_test.bal

@@ -358,3 +358,6 @@ function fooFn() returns future<int> {
 function barFn() returns int {
    return 10;
 }
+
+const int A = 10;
+const int B = 20 + 30;


@sanjana I have a doubt on the original issue (#36831) though.
Say I want to extract the constant. The extracted constant can be either the exact singleton value 30 or it can have the wider basic type node int. As of the current implementation, it only allows int. But @SandaruJayawardana is doing a complete revamp for constant expressions and both have to be allowed. What should be the user experience for this one?

const 30 A = 10 + 20; // valid const int A = 10 + 20; // also valid

The issue is with the expression, right?

I think the behaviour should be consistent for both RHS expressions in

const int A = 10; const int B = 20 + 30;

If the type of 10 is int, it should be the same for 20 + 30 also. But if the type for 20 + 30 is singleton 50, the type for 10 should also be singleton 10. Feels like the singleton approach is more accurate, but since this applies to all literals, may result in a lot of new types being created.

For example, even with int i = 10;, the type of 10 will then be the singleton.

I think we need to discuss this and come to a conclusion and then fix across the compiler for all literals.

Will be holding this PR until the discussion initiated at #38250 is finalized.

github-actions · 2022-11-02T19:39:50Z