Modify

Opened 4 years ago

Closed 4 years ago

#21106 closed defect (fixed)

[patch] False positive: unusual value of turn:lanes with turn:lanes=|right (or anything with no lanes and no `none`)

Reported by: Famlam Owned by: team
Priority: normal Milestone: 21.07
Component: Core validator Version: tested
Keywords: template_report turn:lanes none Cc:

Description

What steps will reproduce the problem?

  1. Validate way 963761819

What is the expected result?

No warning, it has one lane without marking and one that is marked as "right"

What happens instead?

"Unusual value of turn:lanes" (turn:lanes=|right)

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

Regression of #20987. In turn:lanes none can be omitted

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2021-07-12 02:41:41 +0200 (Mon, 12 Jul 2021)
Build-Date:2021-07-12 00:42:49
Revision:18004
Relative:URL: ^/trunk

Identification: JOSM/1.5 (18004 nl) Windows 10 64-Bit
OS Build number: Windows 10 Home 2009 (19043)
Memory Usage: 1679 MB / 1820 MB (407 MB allocated, but free)
Java version: 1.8.0_291-b10, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: nl_NL
Numbers with default locale: 1234567890 -> 1234567890
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (35640)
+ SimplifyArea (35640)
+ imagery_offset_db (35640)
+ pt_assistant (1ff2e15)
+ reverter (35732)
+ tageditor (35640)
+ turnlanes-tagging (288)
+ undelete (35640)
+ utilsplugin2 (35691)

Tagging presets:
+ http://mijndev.openstreetmap.nl/~allroads/JOSM/Presets/NL-Fiets.zip

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/PublicTransport&zip=1
+ %UserProfile%\Documents\tijdelijke bestanden\josm-eigen.mappaint.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Styles/Sidewalks&zip=1

Validator rules:
+ %UserProfile%\Documents\tijdelijke bestanden\josm-eigen.validator.mapcss

Last errors/warnings:
- 00013.187 E: Lokaliseren van afbeelding 'bus.png' mislukt
- 07048.483 W: JCS - Silent failure during download: https://service.pdok.nl/hwh/luchtfotorgb/wms/v1_0?FORMAT=image/jpeg&VERSION=1.3.0&SERVICE=WMS&REQUEST=GetMap&LAYERS=2021_quick_orthoHR&STYLES=&CRS=EPSG:3857&WIDTH=512&HEIGHT=512&BBOX=654224.5250928,6791735.7112510,654300.9621211,6791812.1482793
- 07089.806 W: JCS - Silent failure during download: https://service.pdok.nl/hwh/luchtfotorgb/wms/v1_0?FORMAT=image/jpeg&VERSION=1.3.0&SERVICE=WMS&REQUEST=GetMap&LAYERS=2021_quick_orthoHR&STYLES=&CRS=EPSG:3857&WIDTH=512&HEIGHT=512&BBOX=654300.9621211,6791582.8371945,654377.3991494,6791659.2742228
- 07091.297 W: JCS - Silent failure during download: https://service.pdok.nl/hwh/luchtfotorgb/wms/v1_0?FORMAT=image/jpeg&VERSION=1.3.0&SERVICE=WMS&REQUEST=GetMap&LAYERS=2021_quick_orthoHR&STYLES=&CRS=EPSG:3857&WIDTH=512&HEIGHT=512&BBOX=654300.9621211,6791659.2742228,654377.3991494,6791735.7112510
- 07091.913 W: JCS - Silent failure during download: https://service.pdok.nl/hwh/luchtfotorgb/wms/v1_0?FORMAT=image/jpeg&VERSION=1.3.0&SERVICE=WMS&REQUEST=GetMap&LAYERS=2021_quick_orthoHR&STYLES=&CRS=EPSG:3857&WIDTH=512&HEIGHT=512&BBOX=654300.9621211,6791506.4001662,654377.3991494,6791582.8371945
- 08044.652 W: Verwijderde of verplaatste objecten - <html>Er staan 3 objecten in uw lokale gegevensset die zouden kunnen worden verwijderd op de server.<br>Indien u dit later probeert te verwijderen of bij te werken zal de server zeer waarschijnlijk een melding van een conflict geven.<br>Klik op <strong>Controleer op de server</strong> om de status van deze objecten op de server te controleren.<br>Klik op <strong>Negeren</strong> om te negeren.</html>

Attachments (1)

josm_21106.patch (6.1 KB ) - added by skyper 4 years ago.
patch

Download all attachments as: .zip

Change History (12)

comment:1 by skyper, 4 years ago

Keywords: regression removed

You are right, but I think it is better to add none. Have to update the preset, though, and maybe the message should be changed to reflect the empty value e.g. missing none.

Anyway, it is no regression as the rule was just added.

in reply to:  1 comment:2 by skyper, 4 years ago

Replying to skyper:

Have to update the preset, though,

Oh, I already did that.

comment:3 by Famlam, 4 years ago

Last edited 4 years ago by Famlam (previous) (diff)

comment:4 by Famlam, 4 years ago

Hmm, it's a very widely used, documented scheme to omit none. (Personally I tend to remove nones when I encounter them, they're quite rare compared to ""). So although JOSM can encourage none over "", I don't believe it should flag it as bad.

See https://taginfo.openstreetmap.org/keys/turn:lanes#values

P.s. it wasn't present in the previous rules, so hence the regression. But that's just a word, bug/false positive is enough ;)

Last edited 4 years ago by Famlam (previous) (diff)

in reply to:  4 ; comment:5 by skyper, 4 years ago

Replying to Famlam:

Hmm, it's a very widely used, documented scheme to omit none.

All examples on the wiki include none.

(Personally I tend to remove nones when I encounter them, they're quite rare compared to ).

Please, don't do that.

So although JOSM can encourage none over , I don't believe it should flag it as bad.

Do you mean it should only be an informal warning for empty value parts in turn:lanes[:*]? Then I would rather remove it without any warning for empty values.

See https://taginfo.openstreetmap.org/keys/turn:lanes#values

Yes, it was in my external preset (Presets/LaneAttributes) for a long time until I changed it some weeks ago. Personally, I used to omit none, as-well.
On the other hand, I know that some users did misunderstand it and most of the issues of missing values (other rule) I find in the wild are caused by users removing the last | without any value after it.

P.s. it wasn't present in the previous rules, so hence the regression. But that's just a word, bug/false positive is enough ;)

There was no rule checking on turn values prior to r17935.

All together, the fix/change would be rather easy, though, I'd like to hear some more opinions.

comment:6 by Famlam, 4 years ago

Then I would rather remove it without any warning for empty values.

I would definitely be in favor of removing the warning altogether (and at minimum for splitting it up, as then I can ignore this validator rule while still having the validation part for any real error), but I could live with an info level "warning" about "Prefer 'none' over <nothing> for readability purposes".

On the other hand, I know that some users did misunderstand it and most of the issues of missing values (other rule) I find in the wild are caused by users removing the last | without any value after it.

Which is what validation of lanes=* vs. the number of |s could help with. If desired, it's quite a large chunk (more suitable for Java I guess as then you can just loop over it):

way[lanes=2][/.+:lanes$/=~/^[^\|]*$/],
way[lanes=3][/.+:lanes$/=~/^([^\|]*\|){0,1}[^\|]*$/],
way[lanes=4][/.+:lanes$/=~/^([^\|]*\|){0,2}[^\|]*$/],
way[lanes=5][/.+:lanes$/=~/^([^\|]*\|){0,3}[^\|]*$/],
way[lanes=6][/.+:lanes$/=~/^([^\|]*\|){0,4}[^\|]*$/],
way[lanes=7][/.+:lanes$/=~/^([^\|]*\|){0,5}[^\|]*$/],
way[lanes=8][/.+:lanes$/=~/^([^\|]*\|){0,6}[^\|]*$/],
way[lanes=9][/.+:lanes$/=~/^([^\|]*\|){0,7}[^\|]*$/],
way[lanes:forward=2][/.+:lanes:forward$/=~/^[^\|]*$/],
way[lanes:forward=3][/.+:lanes:forward$/=~/^([^\|]*\|){0,1}[^\|]*$/],
way[lanes:forward=4][/.+:lanes:forward$/=~/^([^\|]*\|){0,2}[^\|]*$/],
way[lanes:forward=5][/.+:lanes:forward$/=~/^([^\|]*\|){0,3}[^\|]*$/],
way[lanes:forward=6][/.+:lanes:forward$/=~/^([^\|]*\|){0,4}[^\|]*$/],
way[lanes:forward=7][/.+:lanes:forward$/=~/^([^\|]*\|){0,5}[^\|]*$/],
way[lanes:forward=8][/.+:lanes:forward$/=~/^([^\|]*\|){0,6}[^\|]*$/],
way[lanes:forward=9][/.+:lanes:forward$/=~/^([^\|]*\|){0,7}[^\|]*$/],
way[lanes:backward=2][/.+:lanes:backward$/=~/^[^\|]*$/],
way[lanes:backward=3][/.+:lanes:backward$/=~/^([^\|]*\|){0,1}[^\|]*$/],
way[lanes:backward=4][/.+:lanes:backward$/=~/^([^\|]*\|){0,2}[^\|]*$/],
way[lanes:backward=5][/.+:lanes:backward$/=~/^([^\|]*\|){0,3}[^\|]*$/],
way[lanes:backward=6][/.+:lanes:backward$/=~/^([^\|]*\|){0,4}[^\|]*$/],
way[lanes:backward=7][/.+:lanes:backward$/=~/^([^\|]*\|){0,5}[^\|]*$/],
way[lanes:backward=8][/.+:lanes:backward$/=~/^([^\|]*\|){0,6}[^\|]*$/],
way[lanes:backward=9][/.+:lanes:backward$/=~/^([^\|]*\|){0,7}[^\|]*$/],
way[lanes:both_ways=2][/.+:lanes:both_ways$/=~/^[^\|]*$/],
way[lanes:both_ways=3][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,1}[^\|]*$/],
way[lanes:both_ways=4][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,2}[^\|]*$/],
way[lanes:both_ways=5][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,3}[^\|]*$/],
way[lanes:both_ways=6][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,4}[^\|]*$/],
way[lanes:both_ways=7][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,5}[^\|]*$/],
way[lanes:both_ways=8][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,6}[^\|]*$/],
way[lanes:both_ways=9][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,7}[^\|]*$/] {
        throwWarning: tr("{0} is larger than the number of lanes in a *:lanes tag", "{0.tag}");
        group: tr("eigen filters");
        assertMatch: "way highway=primary oneway=yes lanes=2 bus:lanes=no";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no|";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no| turn:lanes=left|through|through|right";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no||";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=||no";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|no";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|no|yes";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes|yes";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|yes|no";
        assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes|yes|yes";
        assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|||";
        assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no||";
        assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|||no";
        assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|||no turn:lanes=left|through|through|right";
        assertNoMatch: "way highway=primary oneway=yes lanes=4";
}
way[lanes=1][/.+:lanes$/=~/\|/],
way[lanes=2][/.+:lanes$/=~/^(.*\|){2}/],
way[lanes=3][/.+:lanes$/=~/^(.*\|){3}/],
way[lanes=4][/.+:lanes$/=~/^(.*\|){4}/],
way[lanes=5][/.+:lanes$/=~/^(.*\|){5}/],
way[lanes=6][/.+:lanes$/=~/^(.*\|){6}/],
way[lanes=7][/.+:lanes$/=~/^(.*\|){7}/],
way[lanes=8][/.+:lanes$/=~/^(.*\|){8}/],
way[lanes=9][/.+:lanes$/=~/^(.*\|){9}/],
way[lanes:forward=1][/.+:lanes:forward$/=~/\|/],
way[lanes:forward=2][/.+:lanes:forward$/=~/^(.*\|){2}/],
way[lanes:forward=3][/.+:lanes:forward$/=~/^(.*\|){3}/],
way[lanes:forward=4][/.+:lanes:forward$/=~/^(.*\|){4}/],
way[lanes:forward=5][/.+:lanes:forward$/=~/^(.*\|){5}/],
way[lanes:forward=6][/.+:lanes:forward$/=~/^(.*\|){6}/],
way[lanes:forward=7][/.+:lanes:forward$/=~/^(.*\|){7}/],
way[lanes:forward=8][/.+:lanes:forward$/=~/^(.*\|){8}/],
way[lanes:forward=9][/.+:lanes:forward$/=~/^(.*\|){9}/],
way[lanes:backward=1][/.+:lanes:backward$/=~/\|/],
way[lanes:backward=2][/.+:lanes:backward$/=~/^(.*\|){2}/],
way[lanes:backward=3][/.+:lanes:backward$/=~/^(.*\|){3}/],
way[lanes:backward=4][/.+:lanes:backward$/=~/^(.*\|){4}/],
way[lanes:backward=5][/.+:lanes:backward$/=~/^(.*\|){5}/],
way[lanes:backward=6][/.+:lanes:backward$/=~/^(.*\|){6}/],
way[lanes:backward=7][/.+:lanes:backward$/=~/^(.*\|){7}/],
way[lanes:backward=8][/.+:lanes:backward$/=~/^(.*\|){8}/],
way[lanes:backward=9][/.+:lanes:backward$/=~/^(.*\|){9}/],
way[lanes:both_ways=1][/.+:lanes:both_ways$/=~/\|/],
way[lanes:both_ways=2][/.+:lanes:both_ways$/=~/^(.*\|){2}/],
way[lanes:both_ways=3][/.+:lanes:both_ways$/=~/^(.*\|){3}/],
way[lanes:both_ways=4][/.+:lanes:both_ways$/=~/^(.*\|){4}/],
way[lanes:both_ways=5][/.+:lanes:both_ways$/=~/^(.*\|){5}/],
way[lanes:both_ways=6][/.+:lanes:both_ways$/=~/^(.*\|){6}/],
way[lanes:both_ways=7][/.+:lanes:both_ways$/=~/^(.*\|){7}/],
way[lanes:both_ways=8][/.+:lanes:both_ways$/=~/^(.*\|){8}/],
way[lanes:both_ways=9][/.+:lanes:both_ways$/=~/^(.*\|){9}/] {
        throwWarning: tr("{0} is smaller than the number of lanes in a *:lanes tag", "{0.tag}");
        group: tr("eigen filters");
        assertMatch: "way highway=primary oneway=yes lanes=1 bus:lanes=|no";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=||||no";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|||no|";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=||no||";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no|||";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no||||";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|yes|yes|yes|no";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|yes|yes|no|yes";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|yes|no|yes|yes";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|no|yes|yes|yes";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes|yes|yes|yes";
        assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes|yes|yes|yes turn:lanes=left|through|through|right";
        assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes|yes|yes";
        assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|||";
        assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no||";
        assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|||no";
        assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|||no turn:lanes=left|through|through|right";
        assertNoMatch: "way highway=primary oneway=yes lanes=4";
}

But that's off-topic. Main point in my reply to this part is that a widely used syntax shouldn't be "forbidden", even if some users don't understand it (like many things: you have to learn it first). I guess TagInfo gives enough clues about the opinions of others?

Last edited 4 years ago by Famlam (previous) (diff)

in reply to:  5 comment:7 by Klumbumbus, 4 years ago

Milestone: 21.07

Replying to skyper:

I'd like to hear some more opinions.

There should be no warning for omitted "none" also not on info level.

in reply to:  6 comment:8 by skyper, 4 years ago

Replying to Klumbumbus:

Replying to skyper:

I'd like to hear some more opinions.

There should be no warning for omitted "none" also not on info level.

Ok, empty values need to be added to all turn:lanes*. Below should do the trick (untested)

way[turn:lanes                   ][turn:lanes                    !~ /^(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/],
way[turn:lanes:forward           ][turn:lanes:forward            !~ /^(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/],
way[turn:lanes:backward          ][turn:lanes:backward           !~ /^(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/],
way[turn:lanes:both_ways         ][turn:lanes:both_ways          !~ /^(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/], 
way[turn:lanes:both_ways:forward ][turn:lanes:both_ways:forward  !~ /^(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/],
way[turn:lanes:both_ways:backward][turn:lanes:both_ways:backward !~ /^(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(|none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/] {

Replying to Famlam:

On the other hand, I know that some users did misunderstand it and most of the issues of missing values (other rule) I find in the wild are caused by users removing the last | without any value after it.

Which is what validation of lanes=* vs. the number of |s could help with. If desired, it's quite a large chunk (more suitable for Java I guess as then you can just loop over it):

There is already a test for this problem, Number of lanes greater than *:lanes but it might need some tweaks, see #17172, #18488 and #19649 plus #19095, #19653, and #20236.

Be careful, as only lanes for cars are counted in lanes[:backward/forward/both_ways] but all lanes for *:lanes[:*].

comment:9 by Famlam, 4 years ago

Summary: False positive: unusual value of turn:lanes with turn:lanes=|right (or anything with no lanes and no `none`)[patch] False positive: unusual value of turn:lanes with turn:lanes=|right (or anything with no lanes and no `none`)

Replying to skyper:

Ok, empty values need to be added to all turn:lanes*. Below should do the trick (untested)

I can confirm that patch solves this issue! Thanks Skyper!
Perhaps also add, to prevent future issues:

  assertNoMatch: "way turn:lanes=||through|";

Be careful, as only lanes for cars are counted in lanes[:backward/forward/both_ways] but all lanes for *:lanes[:*].

Thanks!

Last edited 4 years ago by Famlam (previous) (diff)

by skyper, 4 years ago

Attachment: josm_21106.patch added

patch

comment:10 by skyper, 4 years ago

I attached a proper patch file which adds the empty values for turn:lanes* and adjusted the assertNoMatch to include empty values.

comment:11 by Klumbumbus, 4 years ago

Resolution: fixed
Status: newclosed

In 18022/josm:

fix #21106, see #20987 - Don't warn about empty values in turn:lanes tags (patch by skyper)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.