check array rank and dimensions before assignment #3521

gxyd · 2024-02-25T12:55:25Z

gxyd · 2024-02-25T12:58:30Z

I don't fully understand how the below program is valid:

integer function a(n,m)
implicit real (n)
implicit integer (m)
implicit real*8 (k)
integer :: X(m), Y(m)
n = 5
m = 8
X = [1,2,3,4,5]
end function

since we first assign m as 8 and then the size of X (which when initialized as set to m) is 5. This is a test-case in out test-suite.

certik · 2024-02-25T22:08:11Z

That program is invalid. Needs to be fixed.

gxyd · 2024-02-26T10:04:34Z

The failing test from pack (i.e.)

semantic error: Dimensions not equal
 --> /home/runner/work/lfortran/lfortran/integration_tests/array_01_pack.f90:4:3
  |
4 |   p = pack(m, m /= 0)
  |   ^^^^^^^^^^^^^^^^^^^

would be there because of an issue in pack #3155 (comment)

gxyd · 2024-02-26T14:22:30Z

I will put in some more work on this, before someone can review this.

gxyd · 2024-02-27T10:46:10Z

Currently the failing test is:

semantic error: Different shape for array assignment on dimension 1(5 and 2)
  --> /home/runner/work/lfortran/lfortran/integration_tests/intrinsics_51.f90:29:1
   |
29 | cr = sin([[1., 2., 3.], [4., 5.]])
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

as cr is already declared to be of size '5', while we currently don't return sin([[1., 2., 3.], [4., 5.]]) of size '5' (It's a separate bug I believe). I guess this MR has nothing wrong on that part.

gxyd · 2024-02-27T11:04:43Z

This is ready for a review now, let me know how to handle the failing test (if you have any opinion on that).

certik · 2024-02-27T19:50:03Z

integration_tests/valid_array_assignment_same_length_different_start.f90

+    integer :: arr1(2:4)
+    ! array with size 3 and start index 1
+    integer :: arr2(1:3)
+    arr1 = (/1, 2, 3/)


Suggested change

arr1 = (/1, 2, 3/)

arr1 = [1, 2, 3]

Ah, so it seems like [1, 2, 3] is a more modern syntax for arrays followed by Fortran.

certik · 2024-02-27T19:50:31Z

tests/errors/incompatible_dimension_assignment_arr1.f90

+    integer :: arr1(1)
+    ! arr1 (which is of size 1) is being assigned
+    ! an array of size 3
+    arr1 = (/1, 2, 3/)


Suggested change

arr1 = (/1, 2, 3/)

arr1 = [1, 2, 3]

certik · 2024-02-27T19:50:49Z

tests/errors/incompatible_ranks_allocatable_arr1.f90

+
+    ! RHS is a constant array of rank 1
+    ! incompatible assignment
+    arr1 = (/1, 2, 3/)


Suggested change

arr1 = (/1, 2, 3/)

arr1 = [1, 2, 3]

certik · 2024-02-27T19:52:39Z

If your PR uncovered a bug, then comment out the failing lines in tests, and ensure everything passes.

Then we'll decide what to do next.

gxyd · 2024-02-28T08:31:53Z

I've commented out the failing test (which needs a change). Can you please help me understand the error CI error?

certik · 2024-02-28T14:44:22Z

The Windows error is real:

D:\a\lfortran\lfortran\lfortran-0.33.1-532-gccc42db11\src\lfortran\semantics\ast_body_visitor.cpp(2273): error C2146: syntax error: missing ')' before identifier 'and'
D:\a\lfortran\lfortran\lfortran-0.33.1-532-gccc42db11\src\lfortran\semantics\ast_body_visitor.cpp(2273): error C2065: 'and': undeclared identifier
D:\a\lfortran\lfortran\lfortran-0.33.1-532-gccc42db11\src\lfortran\semantics\ast_body_visitor.cpp(2273): error C2146: syntax error: missing ';' before identifier 'dim_b_int'
D:\a\lfortran\lfortran\lfortran-0.33.1-532-gccc42db11\src\lfortran\semantics\ast_body_visitor.cpp(2273): error C2059: syntax error: ')'

certik · 2024-02-28T14:44:52Z

src/lfortran/semantics/ast_body_visitor.cpp

+                    }
+                    ASRUtils::extract_value(ASRUtils::expr_value(dim_a.m_length), dim_a_int);
+                    ASRUtils::extract_value(ASRUtils::expr_value(dim_b.m_length), dim_b_int);
+                    if (dim_a_int > 0 and dim_b_int > 0 && dim_a_int != dim_b_int) {


To fix the Windows error:

Suggested change

if (dim_a_int > 0 and dim_b_int > 0 && dim_a_int != dim_b_int) {

if (dim_a_int > 0 && dim_b_int > 0 && dim_a_int != dim_b_int) {

certik · 2024-02-28T14:46:09Z

integration_tests/intrinsics_51.f90

+!print *, "cr: ", cr
+!do i = lbound(cr, 1), ubound(cr, 1)
+!    if( abs(cr(i) - sin(real(i))) > 1e-6 ) error stop
+!end do


@Pranavchiku can you please have a look at this test and see why it fails? There might be a bug.

MRE:

program intrinsics_51 implicit none real :: cr(5) cr = sin([[1., 2., 3.], [4., 5.]]) print *, "cr: ", cr end program

% lfortran b.f90 --show-stacktrace Traceback (most recent call last): File "/Users/pranavchiku/repos/lfortran/src/bin/lfortran.cpp", line 2391 LCompilers::print_stack_on_segfault(); File "/Users/pranavchiku/repos/lfortran/src/bin/lfortran.cpp", line 2355 err = compile_to_object_file(arg_file, tmp_o, false, File "/Users/pranavchiku/repos/lfortran/src/bin/lfortran.cpp", line 907 result = fe.get_asr2(input, lm, diagnostics); File "/Users/pranavchiku/repos/lfortran/src/lfortran/fortran_evaluator.cpp", line 253 Result<ASR::TranslationUnit_t*> res2 = get_asr3(*ast, diagnostics); File "/Users/pranavchiku/repos/lfortran/src/lfortran/fortran_evaluator.cpp", line 275 compiler_options.symtab_only, compiler_options); File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_to_asr.cpp", line 127 if (res.ok) { File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 3445 b.is_body_visitor = true; File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 143 visit_ast(*x.m_items[i]); File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4848 void visit_ast(const ast_t &b) { visit_ast_t(b, self()); } File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4804 case astType::unit: { v.visit_unit((const unit_t &)x); return; } File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4851 void visit_mod(const mod_t &b) { visit_mod_t(b, self()); } File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4438 case modType::BlockData: { v.visit_BlockData((const BlockData_t &)x); return; } File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 1510 transform_stmts(body, x.n_body, x.m_body); File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 114 this->visit_stmt(*m_body[i]); File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4894 void visit_stmt(const stmt_t &b) { visit_stmt_t(b, self()); } File "/Users/pranavchiku/repos/lfortran/src/lfortran/ast.h", line 4540 case stmtType::Assign: { v.visit_Assign((const Assign_t &)x); return; } File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 2385 check_ArrayAssignmentCompatibility(target, value, x); File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/ast_body_visitor.cpp", line 2277 std::to_string(dim_b_int) + ")", x.base.base.loc); File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/semantic_exception.h", line 20 { } File "/Users/pranavchiku/repos/lfortran/src/lfortran/semantics/semantic_exception.h", line 17 : d{diag::Diagnostic(msg, diag::Level::Error, diag::Stage::Semantic, { semantic error: Different shape for array assignment on dimension 1(5 and 2) --> b.f90:5:5 | 5 | cr = sin([[1., 2., 3.], [4., 5.]]) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Note: Please report unclear or confusing messages as bugs at https://github.com/lfortran/lfortran/issues.

Yes, but the issue is that the RHS should be length 5, not 2. I think this is the case in master as well. So that's the bug, that is unrelated to this PR, that we should investigate.

(IntrinsicScalarFunction Sin [(ArrayPhysicalCast (ArrayConstant [(ArrayConstant [(RealConstant 1.000000 (Real 4) ) (RealConstant 2.000000 (Real 4) ) (RealConstant 3.000000 (Real 4) )] (Array (Real 4) [((IntegerConstant 1 (Integer 4)) (IntegerConstant 3 (Integer 4)))] FixedSizeArray ) ColMajor ) (ArrayConstant [(RealConstant 4.000000 (Real 4) ) (RealConstant 5.000000 (Real 4) )] (Array (Real 4) [((IntegerConstant 1 (Integer 4)) (IntegerConstant 2 (Integer 4)))] FixedSizeArray ) ColMajor )] (Array (Real 4) [((IntegerConstant 1 (Integer 4)) (IntegerConstant 2 (Integer 4)))] FixedSizeArray ) ColMajor ) FixedSizeArray DescriptorArray (Array (Real 4) [((IntegerConstant 1 (Integer 4)) (IntegerConstant 2 (Integer 4)))] DescriptorArray ) () )] 0 (Array (Real 4) [((IntegerConstant 1 (Integer 4)) (IntegerConstant 2 (Integer 4)))] FixedSizeArray ) () --------------------------------------> This should have value, but it does not have )

I faced the same in #3485.

The trouble is eval_Sin is not functioning properly. It should evaluate cr = sin([[1., 2., 3.], [4., 5.]]) the RHS but is not doing.

Yes, I understood the bug, I have been (if possible) looking to make a change to the test case functionality, so that this MR can move forward without necessarily needing the bug to be fixed.

Can you change it to something like:

a = [[1., 2., 3.], [4., 5.]] !print *, a !cr = sin(a)

As well as

cr = sin([1., 2., 3., 4., 5.])

That will test both compile time and runtime and hopefully it works.

@gxyd to make tests pass you just have to skip if value is an IntrinsicElementalFunction or IntrinsicArrayFunction and add a TODO to remove this later

Seems like there is another issue:

> cat b.f90 program main implicit none print *, "rank is:", rank([[1., 2., 3.], [4., 5.]]) print *, "shape is:", shape([[1., 2., 3.], [4., 5.]]) end program > lfortran b.f90 rank is: 1 shape is: 2 > gfortran b.f90 -o b.out && ./b.out rank is: 1 shape is: 5

I can't look into the issue in detail right now, but I'll once I get my monitor back on Saturday (my eyes aren't liking it right now).

gxyd · 2024-03-02T12:42:29Z

I don't understand this new CI error, it's origination from stdlib, I don't understand how stdlib is used in LFortran repo.

certik · 2024-03-02T14:28:25Z

The CI downloads stdlib and compiles it. It fails. It means your new checks uncovered some issues there, that are not caught by our integration tests. We need to understand the issue and write a dedicated integration test for it, and fix it. We can also report it as an issue (to fix later) and workaround the problem in stdlib.

gxyd · 2024-03-02T14:31:09Z

It means your new checks uncovered some issues there

"there" as in "within stdlib"?

certik · 2024-03-02T14:32:13Z

Since stdlib compiles with gfortran, the bug is not in stdlib, it's a bug in LFortran, that is either introduced by this PR, or it has always been in LFortran and only uncovered by this PR's stricter checks.

gxyd · 2024-03-02T14:37:31Z

I see the stacktrace to have

semantic error: Different shape for array assignment on dimension 1(4 and 16)
   --> /home/runner/work/lfortran/lfortran/stdlib/test/linalg/test_linalg.f90:267:9
    |
267 |         c = pack(a, mask)
    |         ^^^^^^^^^^^^^^^^^

which repository contains the file: "test_linalg"? I don't see it on our repo, nor in stdlib.

This PR highlighted (was already known) the issue with pack (as seen in one of the modified integration tests), probably could be similar here.

certik · 2024-03-02T16:43:10Z

See here, which sets up the stdlib at the CI. You can follow the same steps to reproduce this locally:

lfortran/.github/workflows/CI.yml

Line 646 in a9e968b

- name: Test stdlib

There is a link to check out the exact git repository and branch and a hash.

Pranavchiku · 2024-03-03T08:23:56Z

If pack is a blocker, let me know, I'll try to remove the TODO, but that will need to have intrinsic count ( https://gcc.gnu.org/onlinedocs/gfortran/COUNT.html ).

gxyd · 2024-03-05T06:09:30Z

@Pranavchiku , I think to make the tests pass, either it will require changes in the stdlib (in the below code to change c's dimension to n**2:

semantic error: Different shape for array assignment on dimension 1(4 and 16)
   --> /home/runner/work/lfortran/lfortran/stdlib/test/linalg/test_linalg.f90:267:9
    |
267 |         c = pack(a, mask)
    |         ^^^^^^^^^^^^^^^^^

Or the other solution is to actually fix the issue with pack. (I'll re-think for what's a better solution here).

certik · 2024-03-05T06:11:38Z

We can workaround this in stdlib for now, and open up an issue for pack and reference it from there. I think that would be fine. Let's get this PR merged, as it adds many useful checks that uncover bugs.

Pranavchiku · 2024-03-05T06:12:35Z

I am working on implementation of count, will be done by EOD. Then it will be a small change in pack intrinsic.

Pranavchiku · 2024-03-05T19:48:08Z

I have fix for pack almost ready, there are a few cases that fail, will get it done by tomorrow. I am sorry for making you wait to merge this PR.

gxyd · 2024-03-06T05:39:43Z

I am sorry for making you wait to merge this PR.

Don't worry about that. I'm glad that we are doing things the right way.

Pranavchiku · 2024-03-07T05:07:25Z

I have made progress on it, will open PR soon.

Pranavchiku · 2024-03-08T05:30:19Z

With #3605, for p = pack(m, m/=0), node looks like:

                    (Assignment
                        (Var 2 p)
                        (IntrinsicArrayFunction
                            Pack
                            [(ArrayPhysicalCast
                                (Var 2 m)
                                FixedSizeArray
                                DescriptorArray
                                (Array
                                    (Integer 4)
                                    [((IntegerConstant 1 (Integer 4))
                                    (IntegerConstant 6 (Integer 4)))]
                                    DescriptorArray
                                )
                                ()
                            )
                            (ArrayPhysicalCast
                                (IntegerCompare
                                    (Var 2 m)
                                    NotEq
                                    (IntegerConstant 0 (Integer 4))
                                    (Array
                                        (Logical 4)
                                        [((IntegerConstant 1 (Integer 4))
                                        (IntegerConstant 6 (Integer 4)))]
                                        FixedSizeArray
                                    )
                                    ()
                                )
                                FixedSizeArray
                                DescriptorArray
                                (Array
                                    (Logical 4)
                                    [((IntegerConstant 1 (Integer 4))
                                    (IntegerConstant 6 (Integer 4)))]
                                    DescriptorArray
                                )
                                ()
                            )]
                            2
                            (Array
                                (Integer 4)
                                [((IntegerConstant 1 (Integer 4))
                                (IntrinsicArrayFunction
                                    Count
                                    [(ArrayPhysicalCast
                                        (IntegerCompare
                                            (Var 2 m)
                                            NotEq
                                            (IntegerConstant 0 (Integer 4))
                                            (Array
                                                (Logical 4)
                                                [((IntegerConstant 1 (Integer 4))
                                                (IntegerConstant 6 (Integer 4)))]
                                                FixedSizeArray
                                            )
                                            ()
                                        )
                                        FixedSizeArray
                                        DescriptorArray
                                        (Array
                                            (Logical 4)
                                            [((IntegerConstant 1 (Integer 4))
                                            (IntegerConstant 6 (Integer 4)))]
                                            DescriptorArray
                                        )
                                        ()
                                    )]
                                    0
                                    (Integer 4)
                                    ()
                                ))]
                                DescriptorArray
                            )
                            ()
                        )
                        ()
                    )

Pranavchiku · 2024-03-08T05:41:54Z

We cannot get the fixed size at compile time, you'll have to bypass this case.

Pranavchiku · 2024-03-08T06:31:54Z

@gxyd it is merged, I am sorry for delaying this PR. You may rebase it as instructed at #3589 and see if CI works now.

for 'target = value' assignment, when target is: - an allocatable ensure that "rank" is equal of 'target' and 'value' - not an allocatable ensure that "dimension" (includes "rank check") of 'target' and 'value'

gxyd · 2024-03-08T10:56:25Z

Seems like an unrelated failure on Windows

gxyd · 2024-03-08T10:57:44Z

src/lfortran/semantics/ast_body_visitor.cpp

+        ASR::ttype_t *value_type = ASRUtils::type_get_past_allocatable(ASRUtils::expr_type(value));
+        // we don't want to check by "check_equal_type" because we want to allow
+        // real :: x(4); x = [1, 2, 3, 4] to be a valid assignment (as RHS is "integer array")
+        // TODO: the only reason to not do this check for "reshape" is because


I would like to maybe figure out how to fix this error in another PR, I see there are few open issues for "reshape", but I definitely think that this should be made to work.

certik

I think this looks good to merge.

gxyd force-pushed the arrayBug branch from 0d69be7 to 22a9365 Compare February 26, 2024 09:42

gxyd force-pushed the arrayBug branch from d0518a1 to 436dbab Compare February 26, 2024 10:17

gxyd marked this pull request as ready for review February 27, 2024 11:04

certik reviewed Feb 27, 2024

View reviewed changes

gxyd force-pushed the arrayBug branch from 68d932a to d58d30f Compare February 28, 2024 06:59

certik reviewed Feb 28, 2024

View reviewed changes

Pranavchiku mentioned this pull request Feb 28, 2024

Handle ArrayConstant in eval_##X for create_trig in intrinsic functions #3559

Closed

gxyd force-pushed the arrayBug branch from a61ee2e to ecccde0 Compare March 2, 2024 12:12

certik marked this pull request as draft March 2, 2024 14:27

gxyd force-pushed the arrayBug branch from ecccde0 to d380940 Compare March 4, 2024 10:16

certik force-pushed the main branch from bd8e83c to aafa829 Compare March 5, 2024 20:37

Pranavchiku mentioned this pull request Mar 8, 2024

ENH: assign correct dimension in pack #3605

Merged

gxyd added 9 commits March 8, 2024 14:24

check array rank and dimensions before assignment

3a9d9eb

for 'target = value' assignment, when target is: - an allocatable ensure that "rank" is equal of 'target' and 'value' - not an allocatable ensure that "dimension" (includes "rank check") of 'target' and 'value'

fix m_start for lbound

2b94910

fix test case

59026b8

use more specific semantic error message

c4189a9

add some more test cases

140b767

refactor it as a separate function

d5e2f24

comment out invalid test and use modern syntax for array initialization

290a0c6

use '&&' instead of 'and'

d5d7115

don't run failing test case and add a TODO

e4bd07e

gxyd force-pushed the arrayBug branch from d380940 to e4bd07e Compare March 8, 2024 08:59

fix failing errors

9edc954

gxyd marked this pull request as ready for review March 8, 2024 09:22

gxyd marked this pull request as draft March 8, 2024 09:32

remove unncessary code and add comment about "reshape bug"

fa896b2

gxyd marked this pull request as ready for review March 8, 2024 10:32

gxyd commented Mar 8, 2024

View reviewed changes

certik approved these changes Mar 8, 2024

View reviewed changes

certik merged commit 250647a into lfortran:main Mar 8, 2024
21 checks passed

gxyd deleted the arrayBug branch March 8, 2024 17:16

	if (dim_a_int > 0 and dim_b_int > 0 && dim_a_int != dim_b_int) {
	if (dim_a_int > 0 && dim_b_int > 0 && dim_a_int != dim_b_int) {

check array rank and dimensions before assignment #3521

check array rank and dimensions before assignment #3521

Conversation

gxyd commented Feb 25, 2024

gxyd commented Feb 25, 2024

certik commented Feb 25, 2024

gxyd commented Feb 26, 2024

gxyd commented Feb 26, 2024

gxyd commented Feb 27, 2024

gxyd commented Feb 27, 2024

Choose a reason for hiding this comment

gxyd Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

certik commented Feb 27, 2024

gxyd commented Feb 28, 2024 • edited

certik commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

certik Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gxyd commented Mar 2, 2024

certik commented Mar 2, 2024

gxyd commented Mar 2, 2024

certik commented Mar 2, 2024

gxyd commented Mar 2, 2024

certik commented Mar 2, 2024

Pranavchiku commented Mar 3, 2024

gxyd commented Mar 5, 2024

certik commented Mar 5, 2024 • edited

Pranavchiku commented Mar 5, 2024

Pranavchiku commented Mar 5, 2024

gxyd commented Mar 6, 2024

Pranavchiku commented Mar 7, 2024

Pranavchiku commented Mar 8, 2024

Pranavchiku commented Mar 8, 2024

Pranavchiku commented Mar 8, 2024

gxyd commented Mar 8, 2024

gxyd Mar 8, 2024 • edited

Choose a reason for hiding this comment

certik left a comment

Choose a reason for hiding this comment

gxyd Feb 28, 2024 •

edited

gxyd commented Feb 28, 2024 •

edited

certik Feb 28, 2024 •

edited

certik commented Mar 5, 2024 •

edited

gxyd Mar 8, 2024 •

edited