Extend Location manger to handle multi-file import errors #1297

Thirumalai-Shaktivel · 2022-11-14T16:07:29Z

src/libasr/diagnostics.cpp

Thirumalai-Shaktivel · 2022-11-16T05:53:24Z

tests/errors/test__c.py

+from test_mod import i, test
+print(test(5.0))
+print(i)


Works beautifully!
Here is the output:

$ lpython tests/errors/test__c.py --show-asr semantic error: Type mismatch in annotation-assignment, the types must be compatible --> tests/errors/test_mod/test__a.py:3:1 | 3 | i: Const[i32] = 1.23 | ^ ^^^^ type mismatch ('i32' and 'f64') semantic error: The module `test__a` cannot be loaded --> tests/errors/test_mod/__init__.py:1:1 | 1 | from .test__a import i, test | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ imported here semantic error: The module `__init__` cannot be loaded --> tests/errors/test__c.py:1:1 | 1 | from test_mod import i, test | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ imported here Note: if any of the above error or warning messages are not clear or are lacking context please report it to us (we consider that a bug that needs to be fixed).

Now, I'm working on addressing the @certik comments and simplifying things.

Very good, thanks @Thirumalai-Shaktivel !

certik

Otherwise I think this is a very good progress, thanks!

certik · 2022-11-16T22:06:12Z

src/libasr/diagnostics.cpp

@@ -78,6 +81,7 @@ std::string Diagnostics::render(const std::string &input,
        } else {
            throw LCompilersException("Error format not supported.");
        }
+        lm.files.pop_back();


This seems to be using the files from the end of the stack, but I don't think that's a correct assumptions. The locations in a given error message can point to any file.

I agree, I faced this situation while passing the wrong argument for the function

src/libasr/location.h

Thirumalai-Shaktivel · 2022-11-17T07:48:12Z

This PR is ready, kindly review and provide the feedback!
Here is the output
Test 1:

$ lpython tests/errors/test_import_02.py --show-asr
note: The module `test_import_01` cannot be loaded
 --> tests/errors/test_import_02.py:1:1
  |
1 | from test_import_01 import X
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ imported here

note: The module `__init__` cannot be loaded
 --> tests/errors/test_import_01.py:2:1
  |
2 | from test_import import test
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ imported here

note: The module `test_import_1` cannot be loaded
 --> tests/errors/test_import/__init__.py:1:1
  |
1 | from .test_import_1 import test
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ imported here

semantic error: Type mismatch in procedure call; the types must be compatible
 --> tests/errors/test_import_01.py:4:22
  |
4 | X: Const[f64] = test(5.0)
  |                      ^^^ type mismatch (passed argument type is f64 but required type is i32)

 --> tests/errors/test_import/test_import_1.py:3:13
  |
3 | def test(x: i32) -> i32:
  |             ^^^ type mismatch (passed argument type is f64 but required type is i32)


Note: if any of the above error or warning messages are not clear or are lacking
context please report it to us (we consider that a bug that needs to be fixed).

Test 2:

lpython tests/errors/test_import_01.py --show-asr
note: The module `__init__` cannot be loaded
 --> tests/errors/test_import_01.py:2:1
  |
2 | from test_import import test
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ imported here

note: The module `test_import_1` cannot be loaded
 --> tests/errors/test_import/__init__.py:1:1
  |
1 | from .test_import_1 import test
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ imported here

semantic error: Type mismatch in procedure call; the types must be compatible
 --> tests/errors/test_import_01.py:4:22
  |
4 | X: Const[f64] = test(5.0)
  |                      ^^^ type mismatch (passed argument type is f64 but required type is i32)

 --> tests/errors/test_import/test_import_1.py:3:13
  |
3 | def test(x: i32) -> i32:
  |             ^^^ type mismatch (passed argument type is f64 but required type is i32)


Note: if any of the above error or warning messages are not clear or are lacking
context please report it to us (we consider that a bug that needs to be fixed).

Thirumalai-Shaktivel · 2022-11-17T07:58:47Z

How is this handled?

We continue the location of the imported file, i.e., if the new file is imported then the location starts from the size of the previous file. Here are the contents of the filename and file_ends now:

tests/errors/test_import_01.py
tests/errors/test_import/__init__.py
tests/errors/test_import/test_import_1.py
86
118
192

certik · 2022-11-17T14:19:49Z

The CI is getting an error:

/home/runner/work/lpython/lpython/lpython-0.6.0-88-gebd184c09/src/bin/lpython.cpp: In function ‘int {anonymous}::get_symbols(const string&, const string&, LFortran::CompilerOptions&)’:
/home/runner/work/lpython/lpython/lpython-0.6.0-88-gebd184c09/src/bin/lpython.cpp:353:84: error: no matching function for call to ‘LFortran::LocationManager::pos_to_linecol(uint32_t&, uint32_t&, uint32_t&)’
  353 |                lm.pos_to_linecol(a.second->base.loc.first, first_line, first_column);
      |                                                                                    ^

Looks like you need to update some code in lfortran.cpp.

certik · 2022-11-17T14:21:05Z

I think this is roughly correct. Try to get the CI all passing and I'll review very carefully.

Thirumalai-Shaktivel · 2022-11-17T14:36:52Z

The CI is getting an error:

Yup! I fixed everything. Currently, working on the test_error_rendering.cpp

Thirumalai-Shaktivel · 2022-11-17T14:53:53Z

I fixed it, this is ready for review.

Thirumalai-Shaktivel · 2022-11-17T17:21:08Z

src/libasr/utils2.cpp

@@ -25,6 +25,7 @@ bool read_file(const std::string &filename, std::string &text)
    ifs.read(&bytes[0], filesize);

    text = std::string(&bytes[0], filesize);
+    text.erase( std::remove(text.begin(), text.end(), '\r'), text.end() );


This fixes the failure in the windows. I don't know whether this is the correct fix.

The good news is that, this almost fixed the run_tests issue in the Windows.
Ie., Now, we can do python run_tests.py.
The only left out part to fix is to stop the printing this Creating library *.lib and object *.exp in the runtime

We can just filter it from the output on Windows. Do you want to submit a PR?

Ok, this will slow down every file read.

Ideally we handle this in the tokenizer, which we have to extend to handle \r properly.

Why does this change have to be here? What in this PR requires this change?

#1297 (comment)

certik · 2022-11-17T23:03:05Z

src/bin/lpython.cpp

    auto parsing_end = std::chrono::high_resolution_clock::now();
    times.push_back(std::make_pair("Parsing", std::chrono::duration<double, std::milli>(parsing_end - parsing_start).count()));
-    std::cerr << diagnostics.render(input, lm, compiler_options);
+    // std::cerr << diagnostics.render(lm, compiler_options);


I think the errors have to be rendered here:

Suggested change

// std::cerr << diagnostics.render(lm, compiler_options);

std::cerr << diagnostics.render(lm, compiler_options);

certik · 2022-11-17T23:10:40Z

tests/errors/test_import_02.py

@@ -0,0 +1,3 @@
+from test_import_01 import X
+
+print(X)


I think these new tests are not registered in tests.toml?

Here, are the current outputs, I just replaced the note with warning (it highlights with yellow):

$ warning: The module 'test_import/test_import_2' cannot be loaded --> tests/errors/test_import_02.py:1:1 | 1 | from test_import.test_import_2 import X | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ imported here semantic error: Type mismatch in annotation-assignment, the types must be compatible --> tests/errors/test_import/test_import_2.py:3:1 | 3 | X: Const[i32] = 1.23 | ^ ^^^^ type mismatch ('i32' and 'f64') $ semantic error: Type mismatch in procedure call; the types must be compatible --> tests/errors/test_import_01.py:4:22 | 4 | X: Const[f64] = test(5.0) | ^^^ type mismatch (passed argument type is f64 but required type is i32) --> tests/errors/test_import/test_import_1.py:3:13 | 3 | def test(x: i32) -> i32: | ^^^ type mismatch (passed argument type is f64 but required type is i32)

certik · 2022-11-17T23:11:26Z

@Thirumalai-Shaktivel great job. I think this is almost ready. I reviewed every change and I only noticed the issues that I posted above in comments. Let's address them, after which I think this is ready.

Thirumalai-Shaktivel · 2022-11-18T02:09:22Z

src/lpython/tests/test_error_rendering.cpp

+        std::ofstream out("input.txt");
+        out << input;
+        LocationManager::FileLocations fl;
+        fl.in_filename = "input.txt";
+        lm.get_newlines(input, fl.in_newlines);


Here we write the input contents into the input.txt.
In the content "One line text\n": the \n is replaced with \r\n. So, the ctests fail, as \r is added up in the windows. I think, we either remove \r or handle this test_error_rendering separately for windows.
Error:

( semantic error: Error with label no message --> input.txt:1:5 | 1 | One line text | ^^^^ == semantic error: Error with label no message --> input.txt:1:5 | 1 | One line text | ^^^^

I figured out, how to fix this...

Thirumalai-Shaktivel · 2022-11-18T08:14:38Z

src/libasr/diagnostics.cpp

                if (s0.first_line == s0.last_line) {
                    out << std::string(line_num_width+1, ' ') << blue_bold << "|"
                        << reset << std::endl;
                    std::string line = s0.source_code[0];
                    std::replace(std::begin(line), std::end(line), '\t', ' ');
+                    line.erase(std::remove(line.begin(), line.end(), '\r'), line.end());


We currently, use this to fix the \r issue in the error_rendering.

certik · 2022-11-18T23:16:04Z

@Thirumalai-Shaktivel do you want to finish this? Just resolve the conflicts and rebase / merge and this should be good to go.

- Add a new struct FileLocation to store multi- file information. - Add a new vector to store all the file ends. - Now, the error file can be determined using the error location, see the comments under `file_ends` for more details. - Use the index to retrieve all the information like filename, newlines, etc.

…p file. And initialize all the Location information

Store the input into the input.txt. Later use this to render the errors.

Thirumalai-Shaktivel · 2022-11-19T02:51:44Z

I just rebased this on top of sync.
@certik, if everything looks good, please merge this.

czgdp1807

Mind applying libasr/ changes to LFortran's libasr. Reason being this is a very significant change so we should check if it works for LFortran as well. Doing this at the time of next sync will not allow us to test it properly for LFortran. How much time will it take to just apply the libasr/ changes and run the existing tests of LFortran to make sure we are not getting any errors there?

Thirumalai-Shaktivel · 2022-11-19T10:55:06Z

Yup, After merging this PR, I had the same plan to submit a PR to LFortran as well.
I will submit the PR by Monday EOD.

czgdp1807 · 2022-11-19T13:54:32Z

No worries, I will do it on your behalf tomorrow.

czgdp1807 · 2022-11-19T14:30:02Z

Seems like we need to fix lfortran/lfortran#1026 before getting both the PRs into main. Couldn't wait till the next sync to get the changes in LocationManager to LFortran. We need to do it before this goes into main of both the compilers.

@Thirumalai-Shaktivel I have given you push access to my LFortran fork. Please feel free to update lfortran/lfortran#1026 and fix the bugs in it. Then we will merge both the PRs. Thanks.

Thirumalai-Shaktivel · 2022-11-19T15:19:46Z

Thanks, for opening the PR. I'm working on it.

certik

This looks good to me.

@czgdp1807 do you want to first upgrade LFortran before we merge, in lfortran/lfortran#1026?

czgdp1807 · 2022-11-21T04:05:07Z

@czgdp1807 do you want to first upgrade LFortran before we merge, in lfortran/lfortran#1026?

Yes. I would strongly recommend doing so because the changes of libasr don't directly apply to LFortran so it will create problems/blockers for the next sync if not done now. @Thirumalai-Shaktivel I guess you will be able to do it quickly?

Thirumalai-Shaktivel · 2022-11-21T05:34:39Z

Yup, I will complete this by today.

certik · 2022-11-21T22:12:59Z

I reviewed and this looks good, I merged both LFortran and LPython PRs.

Thirumalai-Shaktivel · 2022-11-22T04:07:42Z

Thank you.

Thirumalai-Shaktivel force-pushed the extend_loc_manager_1 branch from d2a6959 to 1cdd5aa Compare November 14, 2022 16:08

Thirumalai-Shaktivel commented Nov 14, 2022

View reviewed changes

src/libasr/diagnostics.cpp Outdated Show resolved Hide resolved

Thirumalai-Shaktivel force-pushed the extend_loc_manager_1 branch 2 times, most recently from b460712 to f3187ba Compare November 16, 2022 05:51

Thirumalai-Shaktivel commented Nov 16, 2022

View reviewed changes

certik mentioned this pull request Nov 16, 2022

Roadmap #155

Closed

9 tasks

certik reviewed Nov 16, 2022

View reviewed changes

Thirumalai-Shaktivel force-pushed the extend_loc_manager_1 branch 3 times, most recently from 5f53ad2 to 4ff2228 Compare November 17, 2022 07:46

Thirumalai-Shaktivel force-pushed the extend_loc_manager_1 branch from 4ff2228 to 934c910 Compare November 17, 2022 14:53

Thirumalai-Shaktivel added the ready for review PRs that are ready for review label Nov 17, 2022

Thirumalai-Shaktivel force-pushed the extend_loc_manager_1 branch 3 times, most recently from ba23349 to 0396bea Compare November 17, 2022 17:19

Thirumalai-Shaktivel commented Nov 17, 2022

View reviewed changes

Thirumalai-Shaktivel force-pushed the extend_loc_manager_1 branch 3 times, most recently from a65de92 to 1b4213a Compare November 17, 2022 18:34

certik marked this pull request as ready for review November 17, 2022 23:00

certik reviewed Nov 17, 2022

View reviewed changes

Thirumalai-Shaktivel commented Nov 18, 2022

View reviewed changes

Thirumalai-Shaktivel force-pushed the extend_loc_manager_1 branch 2 times, most recently from 15c6153 to f8adb68 Compare November 18, 2022 07:07

Thirumalai-Shaktivel changed the title ~~XX: Extend Location manager to handle multiple files~~ Extend Location manger to handle multi-file import errors Nov 18, 2022

Thirumalai-Shaktivel force-pushed the extend_loc_manager_1 branch from f8adb68 to 36df81e Compare November 18, 2022 08:10

Thirumalai-Shaktivel commented Nov 18, 2022

View reviewed changes

Thirumalai-Shaktivel added the enhancement New feature or request label Nov 18, 2022

Thirumalai-Shaktivel added 5 commits November 19, 2022 07:46

Continue the location information for the imported files.

97978bf

Introduce FileLocations to lpython.cpp, ast_to_asr and asr_to_llvm.cp…

7c920d3

…p file. And initialize all the Location information

Introduce FileLocations in the test_error_rendering

3345065

Store the input into the input.txt. Later use this to render the errors.

Add tests and update the refs.

4e1e032

Thirumalai-Shaktivel force-pushed the extend_loc_manager_1 branch from 36df81e to 4e1e032 Compare November 19, 2022 02:50

Erase \r in the line (source_code) to fix Windows CI Failure

5196ecb

czgdp1807 reviewed Nov 19, 2022

View reviewed changes

czgdp1807 mentioned this pull request Nov 19, 2022

Extend Location manger to handle multi-file import errors lfortran/lfortran#1026

Merged

certik approved these changes Nov 20, 2022

View reviewed changes

certik merged commit 93cc7c2 into lcompilers:main Nov 21, 2022

Thirumalai-Shaktivel deleted the extend_loc_manager_1 branch November 22, 2022 04:07

Shaikh-Ubaid mentioned this pull request Nov 24, 2022

Errors from imported files #1294

Closed

	// std::cerr << diagnostics.render(lm, compiler_options);
	std::cerr << diagnostics.render(lm, compiler_options);

Extend Location manger to handle multi-file import errors #1297

Extend Location manger to handle multi-file import errors #1297

Conversation

Thirumalai-Shaktivel commented Nov 14, 2022 • edited Loading

Thirumalai-Shaktivel Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

certik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thirumalai-Shaktivel Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Thirumalai-Shaktivel commented Nov 17, 2022 • edited Loading

Thirumalai-Shaktivel commented Nov 17, 2022 • edited Loading

certik commented Nov 17, 2022

certik commented Nov 17, 2022

Thirumalai-Shaktivel commented Nov 17, 2022

Thirumalai-Shaktivel commented Nov 17, 2022

Choose a reason for hiding this comment

Thirumalai-Shaktivel Nov 17, 2022 • edited Loading

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

Thirumalai-Shaktivel Nov 19, 2022 • edited Loading

Choose a reason for hiding this comment

certik commented Nov 17, 2022

Thirumalai-Shaktivel Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

certik commented Nov 18, 2022

Thirumalai-Shaktivel commented Nov 19, 2022 • edited Loading

czgdp1807 left a comment

Choose a reason for hiding this comment

Thirumalai-Shaktivel commented Nov 19, 2022 • edited Loading

czgdp1807 commented Nov 19, 2022

czgdp1807 commented Nov 19, 2022 • edited Loading

Thirumalai-Shaktivel commented Nov 19, 2022 • edited Loading

certik left a comment

Choose a reason for hiding this comment

czgdp1807 commented Nov 21, 2022

Thirumalai-Shaktivel commented Nov 21, 2022

certik commented Nov 21, 2022

Thirumalai-Shaktivel commented Nov 22, 2022

Thirumalai-Shaktivel commented Nov 14, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 16, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 17, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 17, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 17, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 17, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 19, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 18, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 19, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 19, 2022 •

edited

Loading

czgdp1807 commented Nov 19, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 19, 2022 •

edited

Loading