FWD » Core Development » Conversion Tools

• I only tested a conversion that works (both full and incremental), but I have no idea what happens if something interrupts the entire process with an error, i.e. having an error that stops the conversion entirely. That is something I will look into.
• Also, I only tested Hotel. It will be interesting to see this in a customer application scenario.
• Right now, if you don't set the things up in build.xml, the program gets pretty angry and refuses to run anything. I will look to default to the normal conversion if nothing is set up.
• In terms of performance, I haven't seen any problems. All the conversions that I did (silent, full, original trunk) complete in [2m55s,3m00s]. I will look and see it this affects a customer application, but I don't really expect to.

Radu, javac has plugin support with notifications on what it is doing. This maybe can be used to provide information about the number of compiled classes.

Constantin Asofiei wrote:

Radu, javac has plugin support with notifications on what it is doing. This maybe can be used to provide information about the number of compiled classes.

I'll take a look, thank you.

• Created a listener that (shockingly) listens on Ant's message bus. The party starts when it identifies the Compiling <N> source files to <DIR>.
  • This marks not only the start of the phase, but also the directory where the classes will be, and the total number of classes to be generated.
• The first thing to do is to create a baseline by counting how many class files are already in the directory. Two reasons for this:
  • Javac has two phases, and both generate the class files in the same directory. Without a baseline, the second operation would show inaccurate results right from the start.
  • For incremental conversion, there are classes in the directory already. So it wouldn't be right to recount them again since they won't get recompiled.
• Then, every max(1s, 20 x walk duration), count the number of classes in the directory, compute the delta, and show the result on screen.
  • I put that 20 x walk duration guard so that a project with a lot of classes to compile doesn't slow down just to show a very accurate result. I'll consider this experimental for now, because I haven't tested a large customer application yet. I might change this entirely after a round of testing.

Still, the conclusion is that I will look into that plugin support, maybe it makes the situation easier right from the start.

We should not depend on ant for this solution. ant is just one way that a project can be setup. It is optional and can be replaced with something like gradle or maven or even just not used at all (someone can just write scripts to invoke our tools like ConversionDriver.

Our IDE support will likely be implementing this directly, as an example.

Greg Shah wrote:

We should not depend on ant for this solution. ant is just one way that a project can be setup. It is optional and can be replaced with something like gradle or maven or even just not used at all (someone can just write scripts to invoke our tools like ConversionDriver.

That's actually fine I think. I looked a bit into the plugin that Constantin suggested, and that doesn't require ant stuff. More than that, for the classes, it shows both parsing and compiling progress, and the update happens with more accuracy than what I did. So I'm looking to accommodate this suggestion into the solution.

BTW, I reviewed the videos and I agree this is really nice. We should add the current number of failures to the output.

How is the content rendered when the output is redirected to a file? Also it would be useful to see errors output even in the silence mode. When there are errors typically you want to know what failed.

Here are some notes on the design of the main solution for this task. Radu: this isn't for you, Paula will be implementing this.

• Since the LSP refers to this kind of data as diagnostics AND since the world has too many ErrorManager classes, we will call the main class that manages this data the DiagnosticsManager (DM).
• The DM will manage a collection of DiagnosticsRecord instances.
• Each DiagnosticsRecord will store all the data needed to represent a particular item of note, no matter the severity. This will include details like the location of the problem, the priority level of the issue as well as the details of the problem/error/failure/warning... being reported.
• We will make provisions for the idea that different programmatic users will need to view this collection in different ways. Examples:
  • View the list of errors, sorted by the priority level and then by the order in which the error was encountered in the conversion run.
  • View the list of warnings for the current compilation unit.
  • View all fatal priority records, sorted by record type.
• It is not clear to me if we need to maintain some kind of versioning so that the collection of issues for multiple runs can be managed.

I will create branch 11423b to handle the implementation for #11423-13. I'll leave the a version for Radu so he can wrap up the remaining tasks on his implementation.

Diagnostics infrastructure was committed as rev 16719 (11423b).

Here's what I've done:

Implementation¶

New package com.goldencode.p2j.diagnostic

Lifecycle¶

For now, there is no versioning. A manager belongs to exactly one run: created by the driver, made current through DiagnosticsContext, discarded when the run ends.

Expected problems vs. internal failures¶

Two marker interfaces: Diagnosable (a defect in the input) and InternalFailure (a defect in FWD). Diagnostics walks the whole cause chain:

1. If any link is an Error, an InternalFailure, or one of a short list of JDK runtime exceptions that can only mean a coding mistake, the exception is an internal failure. An internal cause wins over an expected wrapper, so AstException("conversion failed", NullPointerException) stays a FWD defect.
2. Otherwise the innermost Diagnosable or ANTLR link decides, because it is the most specific.
3. Anything matching nothing is an internal failure. Defaulting to internal keeps this honest: a new exception type has to be classified deliberately before it can become a diagnostic.

reportExpected() returns false as an instruction to the caller to keep treating the throwable as the internal failure it is.

reportInternalFailure() records one but does not handle it, the caller still propagates or aborts.

Migrated producers¶

Every migrated site keeps its existing output. Nothing that goes to the console or the cvt log today is changed, removed or duplicated; the manager only collects.

Command line¶

Two options, parsed in the shared TransformDriver.processCommandLine, so ConversionDriver and ProgressTransformDriver both have them.

-diag: is a threshold: a level selects itself and everything more severe. It defaults to none, so an existing build, ant deploy and every other target, produces byte for byte the output it produced before this branch. One constant, TransformDriver.DEFAULT_DIAG_LEVEL, flips that once the output has been lived with.

-diagtype:parse expands to PREPROCESSOR + LEXER + SYNTAX, the three front end stages that turn 4GL text into an AST. COMPILE is deliberately outside parse: those records are also defects in the legacy source, but they are found while annotating and converting the tree, not while parsing it. Ask for parse,compile to get both.

Output is in this form, grouped by compilation unit and closed by a tally:

------------------------------------------------------------------------------
Diagnostics
------------------------------------------------------------------------------

./data/fwd.df
   ./data/fwd.df:37308:3: warning: ignoring unknown option FILE-MISC26 with value "" [schema]
   ./data/fwd.df:146688:2: error: expecting KW_PSC, found 'null' [schema]

4 fatal, 87 error, 1 warning (92 total, 2 shown)

The per level counts always describe the whole run, never the filtered subset, so narrowing the view cannot mislead you into thinking the other problems do not exist. N shown appears only when the filters actually hid something. info and hint are not accepted as thresholds: they are real Severity levels kept for the LSP mapping, but no producer emits them, so offering them would advertise options identical in effect to warning.

Testing¶

For testing, I used the same tests as in #3881 (34 tests)

The checks that matter:

• Type filter isolates. -diagtype:schema reported exactly the 2 schema records and dropped the other 90.
• Union works. -diagtype:schema,internal reported 6 = 2 + 4.
• Threshold works. -diag:error reported 91, precisely one fewer than the baseline, and the run contains exactly one warning — which is the record that disappeared. -diag:fatal selected 4, and those are the same records -diagtype:internal selects, the two filters agreeing from opposite directions.
• Both kinds of silence are unambiguous. -diag:none and a run with no option printed no section at all; a filter that matched nothing printed one line stating what it hid.

Option validation, through the real command line:

-diag:bogus         exit=249  Unknown diagnostic level 'bogus'  Expected one of: none|fatal|error|warning|all.
-diag:info          exit=249  Unknown diagnostic level 'info'  Expected one of: none|fatal|error|warning|all.
-diagtype:nonsense  exit=248  Unknown diagnostic type 'nonsense'  Expected a comma separated list of: all|parse|configuration|schema|preprocessor|lexer|syntax|symbol-resolution|compile|conversion|internal.

Every successful conversion exited 0.

Not covered yet¶

• Middle and back end. Only f2 was run, which is why -diagtype:compile matched nothing.
• Error recovery. #3882's requirement that every error in a file be reported is not implemented; a file still stops at its first unrecoverable point. That needs ANTLR4, and it will probably be resolved in #3882 itself, not here.
• The origin positions of #3881. Locations are recorded as UNMAPPED until that lands.

Open question¶

• Would it make more sense to add the diag and diagtype values directly to directory.xml? By default ant deploy doesn't display the Diagnostics section at the end of the conversion output, but this could be achievable if we set the values directly from directory.xml.

The diagtype compile probably should be called convert and it will be something we want to have be a composite like parse = preprocessor + lexer + syntax. I think we will need types for each phase of the conversion pipeline. I want to reserve compile for real javac failures and equivalent.

Would it make more sense to add the diag and diagtype values directly to directory.xml? By default ant deploy doesn't display the Diagnostics section at the end of the conversion output, but this could be achievable if we set the values directly from directory.xml.

For normal conversion, we never need a directory.xml. I agree that for runtime conversion (e.g. dynamic queries), the directory.xml would be the place to customize it. For normal conversion runs, we should set a good default and then allow the value to be overridden in p2j.cfg.xml.

Greg Shah wrote:

I think we will need types for each phase of the conversion pipeline.

------------------------------------------------------------------------------
Diagnostics
------------------------------------------------------------------------------

.
.
.

./abl/err/pp_fold_dbl.p
   ./abl/err/pp_fold_dbl.p:1:9: error: unexpected token: a [syntax] [phase=2]
   <unknown location>: fatal: Internal FWD failure while processing ./abl/err/pp_fold_dbl.p: com.goldencode.ast.AstException: Error processing ./abl/err/pp_fold_dbl.p [internal] [phase=2]

./abl/err/pp_fold_string.p
   ./abl/err/pp_fold_string.p:5:1: error: unexpected token: DISPALY [syntax] [phase=2]
   ./abl/err/pp_fold_string.p:5:9: error: unexpected token: "fold inside a string: misspelled below the folded literal" [syntax] [phase=2]
   <unknown location>: error: Error processing ./abl/err/pp_fold_string.p [conversion] [phase=2]

./abl/err/pp_fold_token.p
   ./abl/err/pp_fold_token.p:1:1: error: unexpected token: mes [syntax] [phase=2]
   <unknown location>: fatal: Internal FWD failure while processing ./abl/err/pp_fold_token.p: com.goldencode.ast.AstException: Error processing ./abl/err/pp_fold_token.p [internal] [phase=2]

./data/fwd.df
   ./data/fwd.df:37308:3: warning: ignoring unknown option FILE-MISC26 with value "" [schema] [phase=1]
   ./data/fwd.df:146688:2: error: expecting KW_PSC, found 'null' [schema] [phase=1]

<no artifact>
   <unknown location>: fatal: Internal FWD failure while running P2J Conversion Driver: java.lang.RuntimeException: Failed to execute rule-set  [internal] [phase=3]

phase=1: SchemaLoader
phase=2: Scanning Progress Source (preprocessor, lexer, parser, persist ASTs)
phase=3: Post-Parse Fixups

5 fatal, 87 error, 1 warning (93 total)

What do you think of this approach? The diagnostic record now uses a short field named phase instead of storing a full string. This field is resolved into a readable description via a PhaseCatalog interface. Doing it this way allows us to append a legend at the end of the diagnostics, making it easy to identify exactly which phase threw the errors.

Furthermore, for every 4GL file, we should display an additional line that exposes the syntax error precisely as it appears in the cache file. This will typically look like this:

./abl/err/pp_fold_dbl.p
   ./abl/err/pp_fold_dbl.p:ORIG LINE:ORIG COLUMN: error: unexpected token: a [syntax] [phase=2]    ------> if the issue is within an include file, that should be displayed, instead of @./abl/err/pp_fold_dbl.p@
   ./abl/err/pp_fold_dbl.p.cache:1:9: error: unexpected token: a [syntax] [phase=2]    -----> 1 = CACHE LINE, 9 = CACHE COLUMN

What do you think of this approach? The diagnostic record now uses a short field named phase instead of storing a full string. This field is resolved into a readable description via a PhaseCatalog interface. Doing it this way allows us to append a legend at the end of the diagnostics, making it easy to identify exactly which phase threw the errors.

I think there is some merit in the idea. But, if we summarize at that level:

• We may lose imporant information. For example, the phase 2 is far too broad. We would want to know if an issue is in the preprocessor or the lexer or the parser.
• Some tooling in FWD may not fall into this model. The model works for ConversionDriver but we will need diagnostics for other processes too.

Paula Păstrăguș wrote:

Furthermore, for every 4GL file, we should display an additional line that exposes the syntax error precisely as it appears in the cache file. This will typically look like this:

[...]

I'm not sure what this means. Where does that text come from? It doesn't seem to be the 4GL line of code itself (which would be very useful).

Greg Shah wrote:

Paula Păstrăguș wrote:

Furthermore, for every 4GL file, we should display an additional line that exposes the syntax error precisely as it appears in the cache file. This will typically look like this:

[...]

I'm not sure what this means. Where does that text come from? It doesn't seem to be the 4GL line of code itself (which would be very useful).

I manually drafted the output in the pre block to ensure we are aligned. My main point is that when the diagnostics report a parse error, it would be highly beneficial to display the error's exact location in the cache file (cache line and column) right alongside the original location in the .p or .w file.

Yes, we must capture BOTH the original location and the cache file location. The #3881 task was never meant to remove the cache file location, it was meant to add the original location data.

Here are some details about the current diagnostics implementation.

The initial implementation was developed based on the ConversionDriver.. and the needs of the conversion flow, so some of the fields in DiagnosticsRecord are specific to that context. For example, phase represents the job phase that was running when the diagnostic was reported.

One point from the discussion is that DiagnosticsRecord should be generic enough to be reused for both compile-time/convert diagnostics and runtime diagnostics. A runtime diagnostic, for example, does not necessarily have a phase, so phase should not be part of the generic/common diagnostic information.

The common record should therefore focus on information that can be meaningful for any diagnostic, such as the sequence/order, severity, diagnostic type, code, message, source location, and optionally the affected unit, reporting thread, or originating exception (maybe also a timestamp).

If there are any other fields that would be useful for diagnostics, please let me know so I can take them into account in the design.

11424b rev 16720 contains the following:

• Generalized the diagnostics beyond the conversion: removed the phase, which not every diagnostic has;
• Reduced DiagnosticType to eight general kinds, with SYNTAX renamed PARSER and COMPILE dropped;
• Added the record's timestamp and the cache file its position was observed in;
• Made Diagnostics the single static entry point for reporting;
• Defaulted the -diag:/-diagtype: options from p2j.cfg.xml.