FWD » Core Development » Conversion Tools

In conversion and analytics, where we report line/column from cache file we also should report the original unpreprocessed file (proc/class/include) and line/column.

I added some logic to implement the idea above, so the symbols are described with more details in pphints file, for example

&SCOPED-DEFINE num1 message 10000000
&SCOPED-DEFINE num1 10000000
&SCOPED-DEFINE num2 1000000000
&SCOPED-DEFINE num3 100000000000

generates:

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<!--Preprocessor hints--><hints>
  <preprocessor-output>
    <symbol column="1" end-column="29" end-line="1" line="1" name="num1" type="scoped" value="message 10000000"/>
    <symbol column="1" end-column="29" end-line="1" line="1" name="num1" type="scoped" value="10000000"/>
    <symbol column="1" end-column="31" end-line="1" line="1" name="num2" type="scoped" value="1000000000"/>
    <symbol column="1" end-column="33" end-line="1" line="1" name="num3" type="scoped" value="100000000000"/>
  </preprocessor-output>
</hints>

As you can see, all of the symbols have the same starting line, this is because this is with respect to the .cache, so the line is an imaginary one. It is still needed to caclulate line shifts.

The next steps should be I think storing the multi-line scoped and global defines, as well as &if... directives and &message& in a similar fashion, because they also increase the line counter.

Florin Eugen Rotaru wrote:

The next steps should be I think storing the multi-line scoped and global defines,

I worked on this, the pphints now correctly store the starting and ending line for the cases where the definitions is on multiple lines:

&SCOPED-DEFINE num0 message 10000000.~
 message 1.~
 message 1.~
 message 1.~
 message 1.~
 message 2.

&SCOPED-DEFINE num1 message "hello".

&SCOPED-DEFINE num2 message "hello world".

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<!--Preprocessor hints--><hints>
  <preprocessor-output>
    <symbol column="1" end-column="1" end-line="6" line="1" name="num0" type="scoped" value="message 10000000. message 1. message 1. message 1. message 1. message 2."/>
    <symbol column="1" end-column="1" end-line="3" line="2" name="num1" type="scoped" value="message &quot;hello&quot;."/>
    <symbol column="1" end-column="1" end-line="5" line="3" name="num2" type="scoped" value="message &quot;hello world&quot;."/>
  </preprocessor-output>
</hints>

The end-column I think should always be 1, since all of the defines need an empty line as terminators.

I also noticed the possibility to have includes like

{%arg

}

Because this also increases the line discrepancy between the original files and the cache files, I think the pphints should also store this information (same for the {include.i} statements (i.e. the start/end line/col for references, not only the lines where they are written)).

I started working on that and made a first committ to 3881a.

I continued extending the pphints format by adding a child tag for includes and references. Basically, the parent's properties are for the .cache code and the child for the original .p code.

For example:

.p:

&SCOPED-DEFINE num0 message 10000000.~~n~
 message 1.~~n~
 message 1.~~n~
 message 1.~~n~
 message 1.~~n~
 message 2.

{&num0

   }

.cache:

message 10000000.
 message 1.
 message 1.
 message 1.
 message 1.
 message 2.

.pphints:

...
     <symbol column="1" end-column="1" end-line="7" line="1" name="num0" type="scoped" value="message 10000000.~n message 1.~n message 1.~n message 1.~n message 1.~n message 2."/>
    <reference column="1" end-column="12" end-line="7" line="2" name="num0" type="scoped" value="message 10000000.~n message 1.~n message 1.~n message 1.~n message 1.~n message 2.">
      <reference-call end-column="5" line-span="4"/>  // this is the child describes the {&...} call
    </reference>
...

The logic is identical for file includes.

This way, we can map the cache line/col to the original ones in two phases:

1) Expand the preprocessor definitions

2) Shrink the code which was expanded during preprocessor.

Florin Eugen Rotaru wrote:

I have implemented a first working method which computes the original line/column (currently taking into consideration only includes)

{{collapse(Method)
[...]
}}

I will have to test additionally to make sure it works for all cases (e.g. with newlines at the end of the included code, etc). Then I will add similar logic for other possible preprocessor elements.

Last branch revision: 3881a/16024.

I'm continuing to work on the getOriginalLocation() method, I think we can make the code that treats the includes and references generic because they have the same principle: replace the {...} with actual code in the caches.

For this I've created an abstract class called AbstractInjectableHint and made IncludeHint and ReferenceHint extend it.

public abstract class AbstractInjectableHint
{
   private int callStartLine = 0;
   private int callLineSpan = 0;
   private int callEndColumn = 0;

   public int getCallStartLine() 
   {
      return callStartLine;
   }

   public void setCallStartLine(int callStartLine) 
   {
      this.callStartLine = callStartLine;
   }

   public int getCallLineSpan()
   {
      return callLineSpan;
   }

   public void setCallLineSpan(int callLineSpan)
   {
      this.callLineSpan = callLineSpan;
   }

   public int getCallEndColumn()
  {
      return callEndColumn;
   }

   public void setCallEndColumn(int callEndColumn)
   {
      this.callEndColumn = callEndColumn;
   }
}

The call is the actual {...} statement.

With the changes from branch 3881a, one of the next steps is addressing this bug:

for the 4GL code

&SCOPED-DEFINE num0 message 10000000.

{&num0} {&num0} {&num0}

the newly added properties of <reference> and the new tag <reference-call are not very exact if the {&...} are on the same line:

    <reference column="1" end-column="18" end-line="2" line="2" name="num0" type="scoped" value="message 10000000.">
      <reference-call end-column="8" line="3" line-span="1"/>
    </reference>
    <reference column="19" end-column="18" end-line="2" line="2" name="num0" type="scoped" value="message 10000000.">
      <reference-call end-column="31" line="3" line-span="1"/>
    </reference>
    <reference column="37" end-column="18" end-line="2" line="2" name="num0" type="scoped" value="message 10000000.">
      <reference-call end-column="54" line="3" line-span="1"/>
    </reference>

The end-columns are incorrect.

I am also noticing that the tag <symbol name="fwd-version" type="global" value="FWD v4.0.0_undefined_undefined_16003"/> went missing, so this should also be checked at some point.

What I would do next here is:

1. Make sure the bug describe above is fixed (i.e. the pphints metadata correctly stores information about the lines, columns, spans of preprocessor structures)

2. Extend it by adding metadata for &IF, &THEN, &ELSEIF, &ELSE, and &ENDIF preprocessor directives, because these will not be present in the preprocessed files, so we have to know how many lines and columns to retrieve/collapse in order to convert the line/col location.

3. &UNDEFINE preprocessor directive

4. Once those are finished, the getOriginalLocation() describe in #3881-8 method has to consider all these metadata. My intention was to process the @{file.i} and {&reference} calls similarly, in a generic way.

Besides the list from #3881-10, I would also add &ANALYZE-RESUME and &ANALYZE-SUSPEND. Basically, all preprocessor directives need to be taken into account in order to compute the original line and column numbers correctly.

A first commit was made to 3881b / rev 16715.

This rev updates the preprocessor to report output positions based on the original source code. The Environment now records the original file, line, and column for each output line produced, exposing this data via getOriginalLocation().

Also, a few existing position defects had to be fixed along the way. First, the lexer line was consistently off by +1 after returning from an include file. Because this skew accumulated for every include returned into a file, every position reported below an include was wrong. I fixed this by having the preprocessor resume one line earlier at the include exit, which compensates for the artificial line break injected into the resuming file's stream.

I also had to fix a couple of positioning issues in the text.g grammar. String literals were reporting their position just past their closing quote. This happened because CharScanner.setText() calls resetText(), which recaptured the token's start position after the whole literal had already been consumed. Switching to $setText avoids this and properly preserves the position of the opening quote.

Additionally, I had to correct how newlines are attributed when dropping directives like &ANALYZE-SUSPEND and &ANALYZE-RESUME. While the directives are removed from the cache, their newline is preserved. Previously, calling env.print("\n") lacked a token position, forcing the system to fall back on the lexer's live position. Since the lexer reads ahead, the newline was often attributed to the following line. By explicitly passing the token as the position source env.print("\n", nl), the newline is now correctly mapped to the dropped directive's original line without altering the generated cache.

• Multi-line &SCOPED-DEFINE: I still need to analyze Florin's proposed fix to fully understand it.
• Column computation bug: There is a specific case where the original column is calculated incorrectly, yielding a value that is longer than the original line itself.

Paula Păstrăguș wrote:

However there are two remaining issues to address:

• Multi-line &SCOPED-DEFINE: I still need to analyze Florin's proposed fix to fully understand it.

Testcase:

&SCOPED-DEFINE long_msg message "part one" ~
                        "part two" ~
                        "part three".

{&long_msg}

DISPALY "err08: misspelled after a tilde-folded define".

message "after".

• Column computation bug: There is a specific case where the original column is calculated incorrectly, yielding a value that is longer than the original line itself.

Testcase:

&SCOPED-DEFINE bad_stmt def var m as int (.

message "before".

{&bad_stmt}

message "after".

The testcases that were failing yesterday are now passing with rev 16716.

The main issue was how the original column was calculated. Previously, one origin position was kept for each output line, and the original column was calculated by applying an offset from that position. This worked as long as the output line was a direct copy of the source, but it became incorrect when the preprocessor removed or replaced something in the middle of the line. In those cases, the offset between the output and the original source changes, so the calculated column could point to the wrong place or even past the end of the original line.

For macro substitutions, such as {&bad_stmt}, the original column cannot be reliably determined because the expanded text does not actually exist at that position in the source. These lines are therefore recorded as substitution lines, and only the original file and line are reported instead of generating an incorrect column. (Note that in OpenEdge, when a syntax error occurs within a macro, the error message only reports the line number, omitting column information.)

Next, I plan to expand the test coverage. I currently have about 15 test cases, but I want to build a comprehensive test suite to properly validate the solution.

I also noticed that in OpenEdge, syntax errors only report the line number, omitting the column entirely. Additionally, as mentioned previously, we have a specific edge case in our implementation: when an error occurs inside a macro value, we cannot reliably compute the column number. In these instances, our output will similarly report only the line number.

Is there any reason that we should not merge these tests into the testcases project? It will be easier for me to review there.

Also, please update Preprocessor Testcases with the details.

For macro substitutions, such as {&bad_stmt}, the original column cannot be reliably determined because the expanded text does not actually exist at that position in the source.

Don't we record a deletion at the starting location? If there is a hint generated, we should be able to calculate the start column and there is no end column.

For references such as {&bad_stmt}, only the line and column from the cache file corresponding to the expanded value are retained, which might help.

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<!--Preprocessor hints--><hints>
  <preprocessor-output>
    <reference column="1" line="9" name="bad_stmt" type="scoped" value="def var m as int (."/>
    <symbol name="fwd-version" type="global" value="FWD v4.0.0_p2j_11423b_16718"/>
    <symbol name="bad_stmt" type="scoped" value="def var m as int (."/>
  </preprocessor-output>
</hints>

However, in OE, the syntax error is pointing to the line containing the reference itself, not the value, here {&bad_stmt}, not here: &SCOPED-DEFINE bad_stmt def var m as int (. Currently, with 3881b, the original location for that syntax error is pointing as in OE, only the column is not displayed. Please let me know where the original location error should point in this case, and whether it should match OE's behavior in this specific case.

Greg Shah wrote:

Is there any reason that we should not merge these tests into the testcases project? It will be easier for me to review there.

Not exactly, are you referring to the OLD testcases project or the new one?

ALWAYS to the new testcases (Testcases). I've been assuming that you just extended the existing tests.

I'm about to write some new preproc tests myself and would rather do it with your tests already there.

Paula Păstrăguș wrote:

For references such as {&bad_stmt}, only the line and column from the cache file corresponding to the expanded value are retained, which might help.

[...]

However, in OE, the syntax error is pointing to the line containing the reference itself, not the value, here {&bad_stmt}, not here: &SCOPED-DEFINE bad_stmt def var m as int (. Currently, with 3881b, the original location for that syntax error is pointing as in OE, only the column is not displayed. Please let me know where the original location error should point in this case, and whether it should match OE's behavior in this specific case.

Please show the testcase that goes with these pphints.

err06_in_reference.p from the already attached zip file.

/* err06 -- the bad text is the macro's value: written on line 6, used on line
   10.  Either is defensible: the use site (err06_in_reference.p:10) is usually
   what a developer wants, the definition site (line 6) is where the text
   actually lives.  Whichever is reported, it must be one of those two. */

&SCOPED-DEFINE bad_stmt def var m as int (.

message "before".

{&bad_stmt}

message "after".

OK that helps explain the problem.

I see 2 possible cases:

1. The expansion occurs without errors but the error is that the expanded text is invalid 4GL. In this case, we would want to report both the original location of the text that was expanded and well as the location in the cache file where the failure occurred. This may not be a perfect algorithm but that is what I would expect as a developer.
2. The expansion cannot occur because there is no referenced argument/definition by that name. In that case, we should record the location of the reference, which as I said before would certainly have a starting position that we would know.

Tests were committed as rev 1885 into the testcases project.

Can we create a task to keep track of the commits for the testcases and hotel_gui as well?

I noticed there were some obsolete files in the prev rev, so I removed them as rev 1886.

  tests/conversion/preprocessor/test_plan.xml.~1~
  tests/conversion/preprocessor/test_plan.xml.~2~
  tests/conversion/preprocessor/test_plan.xml.~3~
  tests/conversion/preprocessor/test_plan.xml.~4~

Can we create a task to keep track of the commits for the testcases and hotel_gui as well?

It is a good idea. Also a task for Hotel ChUI tracking.

Here's the current testcase that is failing to get the correct original line (the column is correct):

&SCOPED-DEFINE who world

message "{&who ~
          }".

message "{&who
}".

DISPALY "err".

message "after".

The fix was committed as rev 16717.

I am revisiting the solution. If I cannot find any failing tests, I will move this into review.

Revisions 16718 and 16719 are on branch 3881b.

A syntax error now carries the original file, line and column alongside the parser's message, as [originally ...]. The err02 testcase puts five directive lines before the mistake:

//

&SCOPED-DEFINE a 1
&SCOPED-DEFINE b 2
&SCOPED-DEFINE c 3
&GLOBAL-DEFINE d 4
&SCOPED-DEFINE e 5

def var i as int no-undo.
DISPALY "err02: misspelled statement after five defines".

Those five lines vanish, so the parser sees the misspelling at cache line 5. The developer wrote it on line 10, and that is what is also reported now:

unexpected token: DISPALY [originally err02_after_define.p:10:1]
./abl/err/err02_after_define.p:5:1: unexpected token: DISPALY

Where the failing text is a macro's value, the definition is named as well. The err06 testcase defines on line 6 and expands on line 10:

/* err06 -- the bad text is the macro's value: written on line 6, used on line
   10.  Either is defensible: the use site (err06_in_reference.p:10) is usually
   what a developer wants, the definition site (line 6) is where the text
   actually lives.  Whichever is reported, it must be one of those two. */

&SCOPED-DEFINE bad_stmt def var m as int (.

message "before".

{&bad_stmt}

message "after".

unexpected token: ( [originally err06_in_reference.p:10 in {&bad_stmt} defined at ./abl/err/err06_in_reference.p:6:25]

This was achievable by extending the phhints file with def-file, def-line and def-column on the reference and symbol elements.

A new warning is emitted and it covers a reference with no definition. That expands to nothing, which is legal and what OE does. However, for a developer I think it will be very useful to see the warning and fix the typo for example. The err40 testcase has the define spelled max-rows:

&SCOPED-DEFINE max-rows 100

def var i as int no-undo.

assign i = {&max-rowz}.

Warning [./abl/err/err40_undefined_ref.p line 16, col 12]: preprocessor reference {&max-rowz} is not defined, nothing was substituted for it
unexpected token: . [originally err40_undefined_ref.p:16:23]

TBD: I should add the newly added testcases into the testcases project.

Paula Păstrăguș wrote:

[...]

TBD: I should add the newly added testcases into the testcases project.

Done.

I've committed revision 16720: Now the column for a reference is also reported:

For example, for err06 test case, this was reported previously:

unexpected token: ( [originally err06_in_reference.p:10 in {&bad_stmt} defined at ./abl/err/err06_in_reference.p:6:25]

Now, the column value is also reported:

unexpected token: ( [originally err06_in_reference.p:10:1 in {&bad_stmt} defined at ./abl/err/err06_in_reference.p:6:25]

...:10 -> ...10:1

As a final step, I plan to test this against a large application. I'll identify several large 4GL files, inject random syntax errors, and verify that the reported line and column values accurately map back to the original source.

Before I do the review, here is some quick things I'd like you to change:

unexpected token: DISPALY [originally err02_after_define.p:10:1]
./abl/err/err02_after_define.p:5:1: unexpected token: DISPALY

1. Instead of "originally", let's use "source file".
2. Remove the trailing redundant ": unexpected token: DISPALY".
3. Any reference to the cache file location should be listed as the actual cache file name. This would shift "./abl/err/err02_after_define.p:5:1" to "./cvt/err/err02_after_define.p,cache:5:1".

A new warning is emitted and it covers a reference with no definition. That expands to nothing, which is legal and what OE does.

This should not be a warning. It happens very frequently and it is expected. Since OE doesn't show this we don't want to clog log output where OE would not do so. I can see some value to the idea but it should be logged as INFO or maybe even FINE because it is really something that should not be seen normally.

Done as rev 16721.

Here's how it looks rn:

     [java]  [source file pp_fold_string.p:6:9]
     [java] ./cvt/err/pp_fold_string.p.cache:5:9: unexpected token: "fold inside a string: misspelled below the folded literal" 
     [java]     at com.goldencode.p2j.uast.ProgressParser.assignment(ProgressParser.java:9959)
     [java]     at com.goldencode.p2j.uast.ProgressParser.single_block(ProgressParser.java:8575)
     [java]     at com.goldencode.p2j.uast.ProgressParser.block(ProgressParser.java:7660)
     [java]     at com.goldencode.p2j.uast.ProgressParser.external_proc(ProgressParser.java:7587)
     [java]     at com.goldencode.p2j.uast.AstGenerator.parse(AstGenerator.java:1676)
     [java]     at com.goldencode.p2j.uast.AstGenerator.processFile(AstGenerator.java:1067)
     [java]     at com.goldencode.p2j.uast.ScanDriver.lambda$scan$1(ScanDriver.java:436)
     [java]     at com.goldencode.p2j.uast.ScanDriver.scan(ScanDriver.java:476)
     [java]     at com.goldencode.p2j.uast.ScanDriver.scan(ScanDriver.java:305)
     [java]     at com.goldencode.p2j.convert.TransformDriver.runScanDriver(TransformDriver.java:479)
     [java]     at com.goldencode.p2j.convert.TransformDriver.front(TransformDriver.java:334)
     [java]     at com.goldencode.p2j.convert.TransformDriver.executeJob(TransformDriver.java:1198)
     [java]     at com.goldencode.p2j.convert.ConversionDriver.main(ConversionDriver.java:1439)

That is nice. Let's make one more change there.

From:

     [java]  [source file pp_fold_string.p:6:9]
     [java] ./cvt/err/pp_fold_string.p.cache:5:9: unexpected token: "fold inside a string: misspelled below the folded literal" 

to

     [java] ./cvt/err/pp_fold_string.p.cache:5:9 [source file pp_fold_string.p:6:9]: unexpected token: "fold inside a string: misspelled below the folded literal" 

Done as rev 16722.

Greg, the initial results look good... but I discovered a significant oversight. When a file is loaded from the cache, the preprocessor step is skipped entirely. This means we don't have a Preprocessor instance in order to be able to map the original source locations. To get those locations with the current approach, we would have to bypass the cache and run the preprocessor every time, which might not be the best solution:( What do you think?

I think that's why Florin was trying to keep things into the phhints from the very beginning.

Paula Păstrăguș wrote:

Greg, the initial results look good... but I discovered a significant oversight. When a file is loaded from the cache, the preprocessor step is skipped entirely. This means we don't have a Preprocessor instance in order to be able to map the original source locations. To get those locations with the current approach, we would have to bypass the cache and run the preprocessor every time, which might not be the best solution:( What do you think?

I think that's why Florin was trying to keep things into the phhints from the very beginning.

Yes, all details must be in the pphints so they are persisted.

The fix for #3881-43 has been committed as rev 16723.

In other words, should I update every place in Fwd Analytics where a cache position is currently displayed to use the original source location, or are there sections where we intentionally want to preserve the cache position?

Good question. Primarily this is about the "details report" that you showed above, which is where you get after clicking on one of the Category cells.

We don't want to replace or remove the cache file references (or the line/col) in that report. Those are still important and they match up with the source code view that one gets if you click on a cell in the File Name column of that details report.

BUT yet, the original location info is really useful. I'd like to surface it in 2 ways:

1. In the details report. We could consider extra columns but I suspect it would be cramped and confusing. Ideas:

• Add a check-box (off by default) where checking the box will expand normally hidden extra columns for Original File, Original Line, Original Column.
• Add the extra text to the "hover text" for the cells in the File Name column.

I like the first idea better.

2. Include extra columns for Original File, Original Line, Original Column in the CSV export for the details report.

This is good. How reasonable is it to:

• Move the hidden columns to be just after the normal Column, column. This would be for both the details report and the CSV export.
• Have the column labels word wrap or shorten the width (maybe abbreviate them).