[patch] Show warning on invalid MapCSS validator rule parsing

[skyper]

What steps will reproduce the problem?

1. Have a typo in a validator mapcss rule file, like the missing y below:

   way[highway =~ /^(motorway|motorway_link|trunk|trunk_link)$/] {
     set MotorwayTrunk;
   }
   way.MotorwaTrunk[lanes][turn:lanes              ][tag(lanes)!=eval(count(split("|", tag("turn:lanes"))))],
   way.MotorwaTrunk[lanes][change:lanes            ][tag(lanes)!=eval(count(split("|", tag("change:lanes"))))],
   way.MotorwaTrunk[lanes][maxspeed:lanes          ][tag(lanes)!=eval(count(split("|", tag("maxspeed:lanes"))))],
   way.MotorwaTrunk[lanes][minspeed:lanes          ][tag(lanes)!=eval(count(split("|", tag("minspeed:lanes"))))],
   way.MotorwaTrunk[lanes][destination:lanes       ][tag(lanes)!=eval(count(split("|", tag("destination:lanes"))))],
   way.MotorwaTrunk[lanes][destination:ref:lanes   ][tag(lanes)!=eval(count(split("|", tag("destination:ref:lanes"))))],
   way.MotorwaTrunk[lanes][destination:symbol:lanes][tag(lanes)!=eval(count(split("|", tag("destination:symbol:lanes"))))] {
       throwWarning: tr("Different number of lanes in the keys {0} and {1}", "{1.key}", "{2.key}");
       group: tr("suspicious tag combination");
   }
   

2. (Re-)Load file

What is the expected result?

A warning about an unknown class in use.

What happens instead?

No warning

Please provide any additional information below. Attach a screenshot if possible.

Some Information about existing but not use classes would be nice, too.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-08-06 21:47:58 +0200 (Thu, 06 Aug 2020)
Revision:16851
Build-Date:2020-08-07 01:30:47
URL:https://josm.openstreetmap.de/svn/trunk

[gaben]

While experimenting with a custom MapCSS rule, I noticed parsing of invalid rules silently fail on the GUI side. In case of error, the console prints detailed information. Add this or parts to a warning dialog to help diagnose the problem.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2022-01-09 17:28:53 +0100 (Sun, 09 Jan 2022)
Build-Date:2022-01-10 02:31:03
Revision:18364
Relative:URL: ^/trunk

[gaben]

The issue is coming from the MapCSSParser.jj file. I have a patch that makes JOSM print the messages but at the same time introduces some GUI exceptions (EDT violations).

[gaben]

src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSParser.jj

@@ -14 +14 @@
 import java.util.Collections;
 import java.util.List;
 
+import javax.swing.JOptionPane;
+
 import org.openstreetmap.josm.data.preferences.NamedColorProperty;
+import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.gui.mappaint.Keyword;
 import org.openstreetmap.josm.gui.mappaint.Range;
 import org.openstreetmap.josm.gui.mappaint.mapcss.Condition;
@@ -78 +81 @@
             this.idx = idx;
         }
     }
-    
+
     /**
      * Constructor which initializes the parser with a certain lexical state.
      * @param in input
@@ -1164 +1167 @@
     
     Logging.error("Skipping to the next rule, because of an error:");
     Logging.error(e);
+
+    JOptionPane.showMessageDialog(MainApplication.getMainFrame(), e.getMessage(), "MapCSS rule parsing error", JOptionPane.ERROR_MESSAGE);
     if (sheet != null) {
         sheet.logError(e);
     }

The patch introduces some EDT violations, which needs to be solved first. If someone knows how to fix it, please do it.

[GerdP]

See GuiHelper.runInEDT() or one of the variants.

[gaben]

Thank you @GerdP! Now the EDT violations are gone. I think the patch is good enough, but please review it.

[GerdP]

You can simplify that to GuiHelper.runInEDT(() -> JOptionPane.showMessageDialog(..));
like in MainLayerManager:

            GuiHelper.runInEDT(() -> JOptionPane.showMessageDialog(MainApplication.getMainFrame(),
                    tr("Trying to delete the layer with background upload. Please wait until the upload is finished."))); 
}

[gaben]

I tried that, but JavaCC didn't like it :/

[taylor.smock]

Looking at the ​release-notes.md, v7.0.5 "[added] support for Java7 language features". So I don't think Java 8 language features are supported.

[GerdP]

Ah, sorry, I got carried away and forgot to check why. If you think it's useful please commit it. I only tried to help reg. the EDT stuff.

[gaben]

Sorry, I wasn't specific by writing JavaCC doesn't like lambdas, but as taylor.smock wrote, JavaCC supports Java up to version 7.

[taylor.smock]

Replying to GerdP:

Ah, sorry, I got carried away and forgot to check why. If you think it's useful please commit it. I only tried to help reg. the EDT stuff.

I thought you had been looking at the patch. I just figured I'd look into it to see if one of the version updates for JavaCC had added Java 8 language feature support (I think I recently updated our version of JavaCC).

In that case, @gaben, what problem (exactly) are you trying to solve? The supplied patch does nothing to show a warning about a missing class. It just adds another method for informing a user that something might be wrong, and doesn't even trigger on the example mapcss.

Here is some broken mapcss for testing (i.e., what currently happens):

way[highway =~ /^(motorway|motorway_link|trunk|trunk_link)$/] {
  set MotorwayTrunk;
}
/* Should be broken */
way.MotorwaTrunk[lanes][destination:symbol:lanes][tag(lanes)!=eval(count(split("|", tag("destination:symbol:lanes"))))] {
    throwWarning: tr("Different number of lanes in the keys {0} and {1}", "{1.key}", "{2.key}");
    group: tr("suspicious tag combination");
}

/* Broken */
way[hhhh===dddd] {}
way[highway=residential] { throwMessage("Not a valid thing");}
way3 {}

You will notice that the Map Paint Styles toggle dialog will show the paint style with an exclamation mark. Right-click -> Info -> Errors will then show the problems. If you look at the console output, you will see more output (from the Logging calls).

So we need to throw an exception when we encounter a class that does not currently exist.

(Also, JOSM now has a validate command line switch. Example usage: java -jar dist/josm-custom.jar validate --input test.mapcss)

[gaben]

Good catch! I need to revisit what I've done and why, but I remember we worked together with @skyper on a different ticket, and the validator did not warn us about invalid MapCSS rules in runtime, hence the patch. I just ignored the example, probably.

[gaben]

Replying to taylor.smock:

In that case, @gaben, what problem (exactly) are you trying to solve?

If I remember correctly, there is a "hot reload" feature in JOSM, which allows rules reloading simply by saving the rule file. If there is an invalid rule, this feature will stop working silently until JOSM restart. With the patch, at least the user will know there is an issue and why isn't it working (detailed message printed).

[taylor.smock]

Replying to gaben:

If I remember correctly, there is a "hot reload" feature in JOSM, which allows rules reloading simply by saving the rule file. If there is an invalid rule, this feature will stop working silently until JOSM restart. With the patch, at least the user will know there is an issue and why isn't it working (detailed message printed).

Yep, there is a hot reload feature. I haven't seen it stop working after an invalid rule though. If you have an example, we should probably fix that.

[skyper]

Yes, the auto reload does stop working once there is a syntax error (e.g. unexpected symbol). Manually reloading will still work but for auto-reload you need to restart JOSM. (Do not know if it is the same with mappaint styles).

[taylor.smock]

Looks like there is a difference between MapCSSTagChecker.java​ and MapPaintStyles.java​.

This can probably be fixed by moving the FileWatcher#registerSource call above the MapCSSTagChecker#addMapCss call.

EDIT: Note that this appears to only be a problem on _first_ load of the validator.mapcss rule.

src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java

@@ -331 +331 @@
                 } else if (Logging.isDebugEnabled()) {
                     Logging.debug(tr("Adding {0} to tag checker", i));
                 }
-                addMapCSS(i);
                 if (Config.getPref().getBoolean("validator.auto_reload_local_rules", true) && source.isLocal()) {
                     FileWatcher.getDefaultInstance().registerSource(source);
                 }
+                addMapCSS(i);
             } catch (IOException | IllegalStateException | IllegalArgumentException ex) {
                 Logging.warn(tr("Failed to add {0} to tag checker", i));
                 Logging.log(Logging.LEVEL_WARN, ex);

[gaben]

Hm didn't check it but your change should solve the code break, while mine will notify if something is off.