[PATCH][RFC] tag() in validator.mapcss should allow placeholders

[taylor.smock]

What steps will reproduce the problem?

*[/name/=~/.*([‘’]+).*/] {
  throwError: tr("Avoid using unicode '' in names (‘’) ({0})", "{0.key}");
  suggestAlternative: "'";
  fixAdd: concat("{0.key}", "=", tag("{0.key}"), " ", "{0.key}");
  assertNoMatch: "way name=\"Testing's Road\"";
  assertMatch: "way name=\"Testing‘s Road\"";
  assertMatch: "way name=\"Testing’s Road\"";
}

Test way:
way name=\"Testing‘s Road\"

What is the expected result?

I expect the name to become (with the above code) way name=\"Testing's Road name\".

What happens instead?

I get (with the above code) way name=\" name\".

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

It does not appear to be a nested function issue (I checked with nested concats).

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-04-28 04:36:41 +0200 (Sun, 28 Apr 2019)
Revision:15031
Build-Date:2019-04-28 02:37:58
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (15031 en) Mac OS X 10.14.4
OS Build number: Mac OS X 10.14.4 (18E226)
Memory Usage: 375 MB / 2048 MB (116 MB allocated, but free)
Java version: 12+33, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: Display 188945226 1920x1080, Display 69978068 2048x1152
Maximum Screen Size: 2048x1152
Dataset consistency test: No problems found

Plugins:
+ Mapillary (1.5.18)
+ apache-commons (34908)
+ apache-http (34908)
+ auto_tools (73)
+ buildings_tools (34982)
+ continuosDownload (82)
+ graphview (34977)
+ gson (34908)
+ highwayNameModification (1555020960)
+ imagery_offset_db (34908)
+ jna (34908)
+ junctionchecking (34977)
+ kaartvalidation (1554390132)
+ mapdust (${version.entry.commit.revision})
+ markseen (13)
+ openqa (1556288504)
+ osm-obj-info (51)
+ reverter (34977)
+ rex (53)
+ terracer (34977)
+ todo (30306)
+ utilsplugin2 (34977)

Map paint styles:
+ ${HOME}/workspace/osm/JOSM Paint Styles and Presets/Kaart-Styles.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Streets&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Styles/DestinationSignRelation&zip=1
+ https://raw.githubusercontent.com/species/josm-preset-traffic_sign_direction/master/direction.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Styles/Maxspeed&zip=1
+ https://raw.githubusercontent.com/OpenSidewalks/OpenSidewalks-Schema/master/open_sidewalks.mapcss

Validator rules:
+ ${HOME}/workspace/osm/kaart.validator.mapcss

Last errors/warnings:
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5.0 (i.e., TagMap[highway=residential,maxweight=5.0])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5 (i.e., TagMap[highway=residential,maxweight=5])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5.0 (i.e., TagMap[highway=residential,maxweight=5.0])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5 (i.e., TagMap[highway=residential,maxweight=5])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5.0 (i.e., TagMap[highway=residential,maxweight=5.0])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5 (i.e., TagMap[highway=residential,maxweight=5])
- W: Expecting test 'Avoid using unicode ' in names (‘’) ({0.key})' (i.e., [*[name'REGEX'.*([‘’']+).*]]) to not match way name="Testing's Road" (i.e., TagMap[name=Testing's Road])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5.0 (i.e., TagMap[highway=residential,maxweight=5.0])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5 (i.e., TagMap[highway=residential,maxweight=5])
- E: Thread main-worker-0 raised java.lang.IllegalArgumentException: bytes must be >= 0

[Don-vip]

Why don't you use "{0.tag}" instead of tag("{0.key}")?

[taylor.smock]

I'm using a regex instead of a set value. {0.tag} gives name=.*([‘’]+).* and {0.value} gives .*([‘’]+).*. This indicated (to me) that the {0.tag}/{0.value} give the value as written in the test, not in the actual object. Since I'm not the only person writing validator.mapcss checks, I figured that changing the behavior of tag("{0.key}") would be the least likely to break other validator.mapcss checks publicly available (some might depend upon the current behavior of {0.tag}/{0.value}).

[Don-vip]

OK

[simon04]

(Inventor of the nasty placeholders here.) A different approach would be to provide the actual value for {0.value} and {0.tag}.

[taylor.smock]

I haven't looked at this in awhile, but I was worried (at the time), that some other third party validator.mapcss files would do something like this:

[name=~/^[A-Z] ((E|e)ast|(N|n)orth|(W|w)est|(S|s)outh)( [0-9/]+)? .*/],
[name=~/^[A-Z] [a-z]( [0-9/]+)? .*$/],
[name=~/^[A-Z][a-z]?( [0-9/]+)? .*$/] {
  throwError: tr("{0} matches the regex {1}. Check capitalization and spacing.", tag("name"), "{0.value}");
}

I haven't checked that for issues, so there might be syntax errors in there somewhere. (My county uses letters for road names, so E Road, ME Road, ME 3/4 Road, and so on, so I might end up using this to check roads in my local county sometime).

[taylor.smock]

Possible duplicate: #22096

I haven't looked into this, but I rather suspect that fixing one will fix the other.

[taylor.smock]

Add PlaceholderExpression for strings that need to have placeholders replaced

[taylor.smock]

Notes on the patch:

• Add selector field to Environment, uses record-style getter
• Modifies MapCSSParser.jj to use PlaceholderExpression when the placeholder pattern matches. Calling ant javacc after applying this patch is required.
• Where possible, set the selector for the Environment
• Modify whichSelectorMatchesPrimitive(OsmPrimitive primitive) to create a new Environment with a MultiCascade -- this is required to have rules with !.something to not fail the autofix test.
• Update expected toString output in MapCSSTagCheckerTest#testNaturalMarsh (the previous toString output used a string literal instead of an Expression)
• Update numeric.mapcss to use the new functionality. This also changes some things around, partly for simplicity, and partly to better match the OSM unit standard.

EDIT: Additional locations in our source code that requires this functionality (using git grep '/\*.*{\d\+\.\(tag\|key\|value\)}.*\*/' -- resources):

• source:trunk/resources/data/validator/wikipedia.mapcss@18509:57#L57​

[taylor.smock]

Ticket #22096 has been marked as a duplicate of this ticket.

[Famlam]

I can only review a little bit of it, but here's my feedback:

unusual value of {0}: use abbreviation for '' for foot and \" for inches, no spaces

Probably you mean:
unusual value of {0}: use '' for foot and \" for inches, no spaces
or: (but in this case the no spaces is confusing)
unusual value of {0}: use abbreviation or '' for foot and \" for inches, no spaces

---

set separator_autofix;

This separator_autofix isn't used elsewhere, probably it was intended to be on the rules that formerly had !.*_separator_autofix at the end of the selector? Otherwise you get a double warning (one for , instead of ., and one generic one). Probably a similar double warning occurs for a value which has a bad unit.

[taylor.smock]

In 18757/josm:

Fix #17669, #22096: Allow placeholders in more locations in MapCSS