mapcss regexp broken

[Klumbumbus]

What steps will reproduce the problem?

1. add alt_name=bla to a way
2. validate

What is the expected result?

warning "alternative name without" name

What happens instead?

no warning

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

This is a regression of r17593 (r17592 works for me, r17610 doesn't)
It seems the $ in [/_name$/] doesn't work.
This rule in https://josm.openstreetmap.de/browser/josm/trunk/resources/data/validator/combinations.mapcss#L541 doesn't work for me:

*[/_name$/][!name][!old_name][!loc_name][!reg_name][!uic_name][!artist_name][!lock_name][!"osak:municipality_name"][!"osak:street_name"][noname!=yes] {
  throwWarning: tr("alternative name without {0}", "{1.key}");
  group: tr("missing tag");
}

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2022-05-12 16:24:18 +0200 (Thu, 12 May 2022)
Revision:18445
Build-Date:2022-05-13 01:30:54
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (18445 de) Windows 10 64-Bit
OS Build number: Windows 10 Pro 2009 (19044)
Memory Usage: 254 MB / 4058 MB (125 MB allocated, but free)
Java version: 11.0.15+10-LTS, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1680×1050 (scaling 1.00×1.00)
Maximum Screen Size: 1680×1050
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: de_DE
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-Dicedtea-web.bin.location=%UserProfile%\AppData\Local\OpenWebStart\javaws, -Djava.util.Arrays.useLegacyMergeSort=true, --add-exports=jdk.deploy/com.sun.deploy.config=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-reads=java.naming=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.action=ALL-UNNAMED,java.desktop, --add-reads=java.base=ALL-UNNAMED,java.desktop, --add-exports=java.naming/com.sun.jndi.toolkit.url=ALL-UNNAMED,java.desktop, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-exports=java.desktop/com.apple.eawt=ALL-UNNAMED, --add-exports=java.desktop/sun.awt=ALL-UNNAMED,java.desktop, --add-opens=java.base/jdk.internal.ref=ALL-UNNAMED, --add-exports=java.base/sun.security.validator=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/com.sun.imageio.plugins.jpeg=ALL-UNNAMED, --add-exports=java.base/sun.net.www.protocol.jar=ALL-UNNAMED,java.desktop, --add-exports=java.base/jdk.internal.util.jar=ALL-UNNAMED,java.desktop, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, --add-exports=java.base/com.sun.net.ssl.internal.ssl=ALL-UNNAMED,java.desktop, --add-exports=javafx.graphics/com.sun.javafx.application=ALL-UNNAMED, --add-modules=java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web, --add-exports=java.desktop/sun.awt.X11=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/sun.applet=ALL-UNNAMED,java.desktop,jdk.jsobject, --add-exports=java.base/sun.net.www.protocol.http=ALL-UNNAMED,java.desktop, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-exports=java.base/sun.security.util=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/com.sun.imageio.spi=ALL-UNNAMED, --add-reads=java.desktop=ALL-UNNAMED,java.naming, --add-opens=java.base/java.nio=ALL-UNNAMED, --add-exports=java.base/sun.security.x509=ALL-UNNAMED,java.desktop, --add-opens=java.desktop/javax.imageio.spi=ALL-UNNAMED, --add-exports=java.desktop/javax.jnlp=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.provider=ALL-UNNAMED,java.desktop]
Dataset consistency test: No problems found

Plugins:
+ DirectUpload (35951)
+ HouseNumberTaggingTool (35951)
+ OpeningHoursEditor (35924)
+ RoadSigns (35935)
+ SimplifyArea (35893)
+ apache-commons (35924)
+ buildings_tools (35951)
+ editgpx (35931)
+ imagery-xml-bounds (35893)
+ imagery_offset_db (35893)
+ measurement (35893)
+ photo_geotagging (35933)
+ photoadjust (35893)
+ reltoolbox (35893)
+ reverter (35893)
+ tageditor (35893)
+ tagging-preset-tester (35893)
+ terracer (35893)
+ turnlanes-tagging (v0.0.5)
+ turnrestrictions (35893)
+ undelete (35893)
+ utilsplugin2 (35951)

Tagging presets:
+ https://josm.openstreetmap.de/josmfile?page=Presets/COVID-19&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Presets/OneClick&zip=1
+ %UserProfile%\Documents\OSM\TestNew\newpresets.xml
+ https://josm.openstreetmap.de/josmfile?page=Presets/NewTags&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Presets/Xmas&zip=1
+ %UserProfile%\Documents\OSM\josm\core\resources\data\defaultpresets.xml

Map paint styles:
+ %UserProfile%\Documents\OSM\josm\core\resources\styles\standard\elemstyles.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Streets&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/HiDPISupport&zip=1
- %UserProfile%\Documents\OSM\josm\core\resources\styles\standard\potlatch2.mapcss
+ %UserProfile%\Documents\OSM\TestNew\newicons.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Modified&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Maxspeed&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/sac_scale&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/ShowID&zip=1
- %UserProfile%\Documents\OSM\eigene styles\PriorityRoad\PriorityRoad_1.0.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/LayerChecker&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Surface&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/ParkingLanes&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Sidewalks&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Cycleways&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/LitObjects&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Lit&style&zip=1
- %UserProfile%\Documents\OSM\eigene styles\SpecificBuildingValues\SpecificBuildingValues.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Incline&zip=1
- %UserProfile%\Documents\OSM\eigene styles\area-symbol.zip
- https://josm.openstreetmap.de/josmfile?page=Styles/hazmat&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Suburb&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Postcode&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/RecyclingMaterials&zip=1
- %UserProfile%\Documents\OSM\eigene styles\maxspeed\maxspeed_2.9_01 basierend auf 2.7_02 Zahlen.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Bench&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/ColourTag&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Admin_Boundaries&zip=1
- %UserProfile%\Documents\OSM\eigene styles\yes-no-unset\YesNoUnset_1.0.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/PublicTransport&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/FixmeAndNote&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Potlatch2&zip=1

Validator rules:
- https://raw.githubusercontent.com/<user.name>n-a-bauer/josm-validators/master/mtb.validator.mapcss
+ %UserProfile%\Documents\OSM\TestNew\new.validator.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Rules/GermanySpecific&zip=1
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\numeric.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\deprecated.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\combinations.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\geometry.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\unnecessary.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\highway.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\religion.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\multiple.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\addresses.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\wikipedia.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\relation.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\territories.mapcss

Last errors/warnings:
- 00032.995 W: Unable to use English input method
- 00125.459 W: Unable to use English input method
- 00135.970 W: Unable to use English input method
- 00138.007 W: Unable to use English input method
- 00437.829 W: Unable to use English input method
- 00460.415 W: Unable to use English input method
- 00466.760 W: Unable to use English input method
- 00582.598 W: Unable to use English input method
- 03280.300 W: Unable to use English input method

[taylor.smock]

We should probably go through and try to add assertMatch/assertNoMatch tests for everything we have. It looks like we are testing those in source:trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java@HEAD:215-228#L215​.

Anyway, I wonder which branch is actually taken in r17592. It should be taking the 3rd branch.

[Klumbumbus]

The strange thing is, when I add

  assertMatch:   "way alt_name=Foo";
  assertNoMatch: "way alt_name=Foo name=Bar";

I get no warning in the console. But this might be also a shortcoming of the assert tests.

[taylor.smock]

Replying to Klumbumbus:

I get no warning in the console. But this might be also a shortcoming of the assert tests.

If you are talking about the console when you run JOSM (e.g. java -jar josm.jar), you do need to have the advanced option validator.check_assert_local_rules set to true (see wiki:Help/Validator/MapCSSTagChecker#Syntax -- sixth bullet point).

If you are talking about the unit test, then we have a problem. Which we can probably fix by setting the aforementioned configuration setting in the unit test.

[Klumbumbus]

In 18450/josm:

see #22073 - Add asserts

[Klumbumbus]

I mean the java console when I run JOSM, I have validator.check_assert_local_rules set to true.
Can you reproduce the issue(s) described in this ticket?

[taylor.smock]

Replying to Klumbumbus:

I mean the java console when I run JOSM, I have validator.check_assert_local_rules set to true.
Can you reproduce the issue(s) described in this ticket?

I'll check. We might not run it on internal validator rules, but that would be surprising behavior (maybe this got optimized away due to not the unit test?). But I was able to reproduce the original problem with a way highway=road alt_name=FooBar.

[taylor.smock]

It looks like we only check local files, excluding those in jar files with validator.check_assert_local_rules (see source:trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java#L266​ and source:trunk/src/org/openstreetmap/josm/tools/Utils.java#L1184​ (!url.startsWith("resource://"))).

The test I linked to previously seems to be the only way for us to test JOSM internal mapcss validators. Unless we change the code.

I've just stepped through the assertMatch/assertNoMatch code​ with the patch you applied (in that unit test), and it is giving us the expected output (i.e., everything works). I've now stepped through the same code with a debugger, with JOSM starting, and the assertions are failing (I overrode the check for non-local files).

Now to debug what changes between unit tests and the application.

[taylor.smock]

I've found the root of the problem. We are adding the rule as _name to the index, and then when we are iterating through the rules, we look for alt_name in the index. Solution: Don't add it to the index and instead put it in the remaining set. Also, I think I probably messed something up somewhere in comment:7 (when stepping through the code again, the asserts passed in the unit test).

I think we are probably going to want to have an EnumSet of valid KeyMatchTypes. Probably EQ, TRUE, FALSE.

src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSRuleIndex.java

@@ -4 +4 @@
 import java.util.ArrayList;
 import java.util.BitSet;
 import java.util.Collections;
+import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
@@ -120 +121 @@
         }
     }
 
+    /** Valid key types for indexing (see {@link ConditionFactory.KeyMatchType}) */
+    private static final EnumSet<ConditionFactory.KeyMatchType> VALID_INDEX_KEY_TYPES = EnumSet.of(
+            ConditionFactory.KeyMatchType.EQ, ConditionFactory.KeyMatchType.TRUE, ConditionFactory.KeyMatchType.FALSE);
+
     /**
      * All rules this index is for. Once this index is built, this list is sorted.
      */
@@ -186 +191 @@
     private static String findAnyRequiredKey(List<Condition> conds) {
         String key = null;
         for (Condition c : conds) {
-            if (c instanceof KeyCondition) {
+            if (c instanceof KeyCondition && VALID_INDEX_KEY_TYPES.contains(((KeyCondition) c).matchType)) {
                 KeyCondition keyCondition = (KeyCondition) c;
                 if (!keyCondition.negateResult) {
                     key = keyCondition.label;

Now to writing a unit test for this.

[taylor.smock]

In 18451/josm:

Fix #22073: Optimized regexes (string starts with, ends with, contains) do not work

This was caused by taking a portion of a regex (for example, _name from
/_name$/), and using that to match against keys. So alt_name would not
match, but _name would. All regex patterns should now be added to the
remaining field in MapCSSRuleIndex, and there now exists a test to ensure
we do not forget to add new non-regex types to the valid key type list.

[Klumbumbus]

Thanks for fixing!

[taylor.smock]

I'd say no problem, but it was a PITA to debug.

But thank you for merging various tag patches (I presume this is how you found this bug). I've been meaning to look at those and merge/give feedback, as needed. It doesn't help that I've tried to follow the history of tag changes/additions when a request to make a change is done (most recently #22069) just so I'm not merging something that was quietly changed yesterday.

[Klumbumbus]

Replying to taylor.smock:

(I presume this is how you found this bug).

Yes, #22033 and first I thought I made some kind of dumb mistake, because the test seemed to be broken for 14 months, but #22033 was created only 4 weeks ago...