Skip to content

[FEATURE] Support for indentation in 'of/else/else if' etc. #18798

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

Closed
wants to merge 4 commits into from
Closed

[FEATURE] Support for indentation in 'of/else/else if' etc. #18798

wants to merge 4 commits into from

Conversation

haxscramper
Copy link
Contributor

Implements nim-lang/RFCs#420

@Araq
Copy link
Member

Araq commented Sep 10, 2021

Needs a gramar update too. For this, patch some #| comment in parser.nim and then run nim c-r compiler/parser.nim.

@Araq Araq added the merge_when_passes_CI mergeable once green label Sep 10, 2021
@Araq
Copy link
Member

Araq commented Sep 10, 2021

There seems to be some nimble important_packages hiccups.

@haxscramper
Copy link
Contributor Author

haxscramper commented Sep 10, 2021

CI passed earlier with the same implementation, and it is very unlikely that grammar comment modification affected semcheck for a package (code fails with type mismatch error).

Does this mean I need to figure out what is wrong, or PR will still be accepted?

@Araq
Copy link
Member

Araq commented Sep 10, 2021

It means we'll close and reopen the PR this weekend until it's green.

@ringabout
Copy link
Member

ringabout commented Sep 11, 2021

CI passed earlier with the same implementation

CI is broken which doesn't run at all but is green until this PR: #18811 (make sure you already fetch the latest commit)

@ringabout ringabout closed this Sep 11, 2021
@ringabout ringabout reopened this Sep 11, 2021
@Araq Araq closed this Sep 11, 2021
@Araq Araq reopened this Sep 11, 2021
@ringabout
Copy link
Member

ringabout commented Sep 11, 2021

I don't think closing and reopening this PR helps. As I said before, the PR may break some packages (CI may be green wrongly before). So make sure the PR already bases on the latest commit. If it were, maybe you need to figure out why the cligen library is broken.

@haxscramper
Copy link
Contributor Author

haxscramper commented Sep 11, 2021

Separately executing testament for package batch 1_3, running inim tests (cligen is shown in CI, but inim test fails) with new compiler showed no issues.

@ringabout
Copy link
Member

it is very unlikely that grammar comment modification affected semcheck for a package

IMO your PR can affect code like this

readStmt = if typeMapping.hasKey(node.protoType): quote do: `stream`.`protoRead`()
  else: quote do: `stream`.`protoRead`(`stream`.protoReadInt64()) #TODO: This is not implemented on the writer level

see protobuf failure

home/runner/work/Nim/Nim/pkgstemp/protobuf/src/protobuf.nim(786, 79) Error: attempting to call routine: 'protoRead'
    found 'protoRead' [let declared in /home/runner/work/Nim/Nim/pkgstemp/protobuf/src/protobuf.nim(785, 11)]

@haxscramper
Copy link
Contributor Author

Closing because it seems like it is impossible to implement cleanly, considering all of these ridiculous edge cases that I didn't even know were valid. Basically allowing any kind of flexibility with block arguments would create 'danging else' kind of situation, turning implementation in a completely broken piece of garbage, and it is not worth it.

  readStmt = if 1: quote do: 3 # ok now
    else: quote do: 2

  readStmt = if 1: quote: 3 # ok now
    else: quote: 2

  match: # Supported now
    myOf: 12
  of:
    discard

  match 1: # supported now, would be ambigous to implement
    match 2:
    of 1: discard # For inner one, indented against 'match 2'
    of 1: discard # Meant to be added to outer, or inner?
  of 1: discard # Added to outer one

The same goes for match arg\nof - very ambiguous, current grammar is overly flexible, someone probably already depends on all edge cases that exist, and breaking their code for minor consistency improvements does not make any sense.

@haxscramper
Copy link
Contributor Author

And this is off-topic, but what is the point of having CI that randomly allows passing invalid code in the first run? If I initially updated grammar, it could potentially be merged, because the implementation looks so "harmless" and "simple" and no sane person could make up code like this for unit tests.

@ringabout
Copy link
Member

ringabout commented Sep 11, 2021

It was a mistake in testament logics and has been fixed.

see #18805

Because it didn't raise when a package name was wrong. The error was swallowed silently.

https://github.com/nim-lang/Nim/pull/18806/files

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants