Rule skipped if `set` used for different type (node/way/relation)

[Famlam]

I tried to update the mapcss files in preparation for another PR, and saw the following line:
# Skip selector using undeclared class TunnelCulvertWithoutWaterway
in the py file for https://josm.openstreetmap.de/browser/josm/trunk/resources/data/validator/combinations.mapcss

It came from this rule:
# *[tunnel][!highway][!area:highway][!railway][!waterway][!piste:type][type!=tunnel][public_transport!=platform][route!=ferry][man_made!=pipeline][man_made!=goods_conveyor][man_made!=wildlife_crossing][man_made!=tunnel][power!=cable]!.TunnelCulvertWithoutWaterway
(which would match 1998 nodes if parsed correctly)

TunnelCulvertWithoutWaterway is however declared, just not for node/relation:

way[tunnel=culvert][man_made!=tunnel][!waterway] {
  throwWarning: tr("{0} without {1} or {2}", "{0.tag}", "{1.tag}", "{2.key}"); 
  group: tr("suspicious tag combination"); 
  set TunnelCulvertWithoutWaterway; 
}

As it is only set for way rules, the mapcss2osmose script now discards any node/relation rule that refers to it via !.TunnelCulvertWithoutWaterway

(The discarding was done in case it was declared in an "unparsable" rule)

[frodrigo]

!.TunnelCulvertWithoutWaterway should be consisted as true on non declared type, or the rule "unparsable" for the undeclard type ?

Please, can you check what JOSM does in this case?

[Famlam]

In JOSM, the following rule matches everything, even though I have never declared any abcdef

*!.abcdef {
  throwWarning: "x";
}

So indeed, .something is always false (and !.something is always true) until you've encountered a set something line.

In fact, the current implementation in Osmose for unparsable rules (which JOSM doesn't have as they define the rules) has another related flaw, as it only checks if a set has been declared once. However, if you have (extreme example) the following:

node[x=y] {
  set abc; /* matches almost nothing, but is parsable by Osmose */
}
way > * {
  set abc; /* matches any node of a way, but can't be parsed by Osmose */
}
node:unconnected {
  set abc; /* matches any node not part of a way, but can't be parsed by Osmose */
}
node!.abc {
  throwWarning: "X";
}

Then Osmose will throw a warning for all nodes, except nodes with x=y, while JOSM won't do anything.

Probably the best is not to filter out rules that have an undeclared set, but only to filter out rules where the set was declared in a rule that could not be parsed.

[frodrigo]

Probably the best is not to filter out rules that have an undeclared set, but only to filter out rules where the set was declared in a rule that could not be parsed.

Yes. I agree with that.

[Famlam]

It still needs to be converted into a proper automated test case, but for manual testing, the following file could perhaps be useful:

File with test cases that should be filtered out and a few good cases that should not be filtered out

area[test] {
  set area1;
}
way[test],
area[test] {
  set area2;
}

*.undeclaredClass {
  set undeclared1;
}
way[test],
*.undeclaredClass {
  set undeclared2;
}

*[test] < *[test] {
  set complex1;
}
way[test],
*[test] < *[test] {
  set complex2;
}

*[test] >[index=1] *[test] {
  set complex3;
}
way[test],
*[test] >[index=1] *[test] {
  set complex4;
}

*:unconnected {
  set unsupportedPseudo1;
}
way[test],
*:unconnected {
  set unsupportedPseudo2;
}
*!:in-downloaded-area {
  set unsupportedPseudo3;
}

*[areasize()] {
  set unsupportedFun1;
}

*[JOSM_search()] {
  set unsupportedFun2;
}

*[parent_tags()] {
  set unsupportedFun3;
}
way[test],
*[parent_tags()] {
  set unsupportedFun4;
  set unsupportedFun5;
}

area[test] {
  set set_twice_once_unsupported1;
}
way[test] {
  set set_twice_once_unsupported1;
}

area[test] {
  set set_discarded_rule1;
  set set_discarded_rule2;
}
*.set_discarded_rule1 {
  set set_in_rule_using_discarded_set1;
}
*.set_discarded_rule2 {
  set set_in_rule_using_discarded_set2;
}

node[test], way.area1 {throwWarning: "way rule should be filtered";}
node[test], way!.area1 {throwWarning: "way rule should be filtered";}
node[test], way.area2 {throwWarning: "way rule should be filtered";}
node[test], way.undeclared1 {throwWarning: "way rule should be filtered";}
node[test], way!.undeclared1 {throwWarning: "way rule should be filtered";}
node[test], way.undeclared2 {throwWarning: "way rule should be filtered";}
node[test], way.complex1 {throwWarning: "way rule should be filtered";}
node[test], way!.complex1 {throwWarning: "way rule should be filtered";}
node[test], way.complex2 {throwWarning: "way rule should be filtered";}
node[test], way.complex3 {throwWarning: "way rule should be filtered";}
node[test], way!.complex3 {throwWarning: "way rule should be filtered";}
node[test], way.complex4 {throwWarning: "way rule should be filtered";}
node[test], way.unsupportedPseudo1 {throwWarning: "way rule should be filtered";}
node[test], way!.unsupportedPseudo1 {throwWarning: "way rule should be filtered";}
node[test], way.unsupportedPseudo2 {throwWarning: "way rule should be filtered";}
node[test], way.unsupportedPseudo3 {throwWarning: "way rule should be filtered";}
node[test], way.unsupportedFun1 {throwWarning: "way rule should be filtered";}
node[test], way!.unsupportedFun1 {throwWarning: "way rule should be filtered";}
node[test], way.unsupportedFun2 {throwWarning: "way rule should be filtered";}
node[test], way.unsupportedFun3 {throwWarning: "way rule should be filtered";}
node[test], way.unsupportedFun4 {throwWarning: "way rule should be filtered";}
node[test], way.unsupportedFun5 {throwWarning: "way rule should be filtered";}
node[test], way.set_twice_once_unsupported1 {throwWarning: "way rule should be filtered";}
node[test], way!.set_twice_once_unsupported1 {throwWarning: "way rule should be filtered";}
node[test], way.set_in_rule_using_discarded_set1 {throwWarning: "way rule should be filtered";}
node[test], way.set_in_rule_using_discarded_set2 {throwWarning: "way rule should be filtered";}
node[test], way!.set_in_rule_using_discarded_set1 {throwWarning: "way rule should be filtered";}



/* Supported cases */
way[test] {
  set valid1;
}
*[test]!.valid1 {
  throwWarning: "I'm an ok rule";
}
*[test].valid1 {
  throwWarning: "I'm an ok rule";
}

way {
  throwOther: "x";
	set throwOtherRule;
}
*!.throwOtherRule {
  throwWarning: "I'm an ok rule";
}

* {
  set a;
	set b;
}
way.a {
  throwWarning: "I'm an ok rule";
}
node.b {
  throwWarning: "I'm an ok rule";
}

Unfortunately I haven't been able to get it working properly yet.

[Famlam]

We have a new, comparable, case:
https://josm.openstreetmap.de/browser/josm/trunk/resources/data/validator/numeric.mapcss
For this rule:

*[min_height ][min_height  !~ /^(-?([0-9]+(\.[0-9]+)?( m)?)|(-?[1-9][0-9]*\'((10|11|[0-9])((\.[0-9]+)?)\")?))$/]!.min_height_separator_autofix!.min_height_meter_autofix!.min_height_foot_autofix {
  throwWarning: tr("unusual value of {0}: {1} is default; point is decimal separator; if units, put space then unit", "{0.key}", tr("meters"));
  assertMatch: "node min_height=\"12. m\"";
  assertNoMatch: "node min_height=-5";
}

the selector min_height_foot_autofix has never been defined. Since it's a negated selector, it works fine in JOSM. For us, the rule gets:

# Skip selector using undeclared class min_height_foot_autofix, min_height_meter_autofix, min_height_separator_autofix

(which by the way is slightly misleading: only min_height_foot_autofix is undeclared).

I'll blacklist the rule and add min_height to Number.py instead; just filing it here for reference. EDIT: this rule will probably disappear again in the commit of https://josm.openstreetmap.de/ticket/17669