Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix RST parsing when no indent after enum.item (fix #17249) #17257

Merged
merged 9 commits into from
Mar 12, 2021

Conversation

a-mr
Copy link
Contributor

@a-mr a-mr commented Mar 4, 2021

According to RST spec this fragment

1. Nim is a statically typed compiled systems
programming language.

should be parsed as normal paragraph, not an enum.list, because there is no necessary indentation on the second line.

This PR makes rst.nim handle it in compliance with the spec and also adds a warning with suggestions how to fix it. Original python rst does not emit any warning but I think it's necessary since this behavior confuses people that are used to Markdown.

  • also change docs in 2 other places appropriately
  • fix that rst2html started numeration of lines from 0 - now it will be from 1 as in all text editors. It should have been done long ago. In this PR it's essential since human interpretation of the warning depends on proper line numbers

cc @narimiran

EDIT(timotheecour) fix #17249 (needed to put in body otherwise this PR won't show up in the ref'd issue: the link in title isn't enough) /cc @a-mr

compiler/docgen.nim Outdated Show resolved Hide resolved
@a-mr
Copy link
Contributor Author

a-mr commented Mar 5, 2021

Digging into line numbers reporting I observed that columns are also wrong but this time they are shifted by +1 (it was not clear because columns are much more often reported completely incorrectly because of more p.idx movement sensitivity).

It's because Nim global error/warning reporting adds ColOffset=1, while there is no such addition for lines. So before rst was parsed with initial line=0, column=1 but it should be line=1, column=0.

@timotheecour
Copy link
Member

let's followup in timotheecour#638 for how to solve this aspect cleanly throughout nim compiler in followup work

lib/pure/strutils.nim Outdated Show resolved Hide resolved
@a-mr
Copy link
Contributor Author

a-mr commented Mar 10, 2021

CI failure is unrelated.

Test improvements will be done in a follow-up PR.

@a-mr a-mr force-pushed the rst-enumlist-continuation-after-nl branch from aa74473 to e13174c Compare March 10, 2021 21:08
let col = currentTok(p).col
var w = 0
while w < wildcards.len:
if match(p, p.idx, wildcards[w]): break
inc w
assert w < wildcards.len
proc checkAfterNewline(p: RstParser, report: bool): bool =
let j = tokenAfterNewline(p, start=p.idx+1)
if p.tok[j].kind notin {tkIndent, tkEof} and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting there, it's almost perfect...

another regression:

1. hello 1
1. hello 2
   foo 2
1. hello 3
   foo 3
   1. sub1
   1. sub2
   1. sub3
   zook5
1. hello 4
1. hello 5
1. hello 6
 zook6
1. hello 7

1. hello 8


1. hello 9

1. hello 10

separator

1. hello 11
1. hello 12

before PR

image

after PR

image

  • sub3 gets displaced
  • there shouldn't be an interruption in numbering form 1. hello 5 to 1. hello 6

but this could potentially be addressed in future PR, wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is working as expected here.

  • sub3 is not displaced. It's just parsed as normal paragraph according to RST specification because there is zook5 without indent after it
  • the same here: "1. hello 6" was parsed as a normal paragraph and zook6 became a block quote because it's indented by 1

And the warning is issued in both cases, so it looks ok to me.

rst2html.py does the same thing if you change 1s to #s.

Also note that presence of auto-numerator 1 does not mean that it's a real "markdown mode".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible to implement also markdown-style enumeration list parsing because it's basically just relaxation of the RST kind. But it would be another task.

@timotheecour
Copy link
Member

timotheecour commented Mar 10, 2021

nim-lang.Nim (packages Linux_i386) Failing after 58s — packages Linux_i386 failed
failed so i restarted it but it failed again (quickly, after 1mn)

can you rebase against devel maybe?

not sure if it relates to your PR

https://dev.azure.com/nim-lang/255dfe86-e590-40bb-a8a2-3c0295ebdeb1/_apis/build/builds/14075/logs/38

2021-03-10T21:10:24.0269335Z CC: stdlib_assertions.nim
2021-03-10T21:10:24.0288424Z CC: stdlib_dollars.nim
2021-03-10T21:10:24.1410956Z Error: execution of an external compiler program 'gcc -c  -w -fmax-errors=3  -I/home/vsts/work/1/s/lib -I/home/vsts/work/1/s -o /home/vsts/.cache/nim/koch_d/stdlib_assertions.nim.c.o /home/vsts/.cache/nim/koch_d/stdlib_assertions.nim.c' failed with exit code: 1
2021-03-10T21:10:24.1412125Z 
2021-03-10T21:10:24.1412666Z In file included from /usr/include/limits.h:25:0,
2021-03-10T21:10:24.1413573Z                  from /usr/lib/gcc/x86_64-linux-gnu/5/include-fixed/limits.h:168,
2021-03-10T21:10:24.1414497Z                  from /usr/lib/gcc/x86_64-linux-gnu/5/include-fixed/syslimits.h:7,
2021-03-10T21:10:24.1415407Z                  from /usr/lib/gcc/x86_64-linux-gnu/5/include-fixed/limits.h:34,
2021-03-10T21:10:24.1416047Z                  from /home/vsts/work/1/s/lib/nimbase.h:268,
2021-03-10T21:10:24.1416680Z                  from /home/vsts/.cache/nim/koch_d/stdlib_assertions.nim.c:9:
2021-03-10T21:10:24.1417570Z /usr/include/features.h:367:25: fatal error: sys/cdefs.h: No such file or directory
2021-03-10T21:10:24.1418154Z compilation terminated.
2021-03-10T21:10:24.1472789Z ##[error]Bash exited with code '1'.
2021-03-10T21:10:24.1576588Z ##[section]Finishing: Build koch

(refs #3123 (comment))
refs:

@timotheecour
Copy link
Member

timotheecour commented Mar 10, 2021

good news, i have same error in another PR i just rebased (#17304) so this isn't related to your PR (=> issue #17325)

Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, remaining points can be addressed in followup work

@timotheecour
Copy link
Member

(closed/re-opened so that bugfix #17327 is included)

@narimiran narimiran merged commit d97bf4f into nim-lang:devel Mar 12, 2021
@timotheecour timotheecour mentioned this pull request Mar 17, 2021
28 tasks
timotheecour added a commit to timotheecour/Nim that referenced this pull request Apr 29, 2021
Araq pushed a commit that referenced this pull request Apr 30, 2021
…17895)

* revive #16627 now that csources_v1 was merged

* use dedent in rst.nim, refs #17257 (comment)

* fix test and improve rendering of a rst warning
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
…ces_v1 (nim-lang#17895)

* revive nim-lang#16627 now that csources_v1 was merged

* use dedent in rst.nim, refs nim-lang#17257 (comment)

* fix test and improve rendering of a rst warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

regression (1.4 => devel): rst: paragraph incorrectly created on line break, in 1st line of a list
4 participants