Error on indents #9855
Error on indents #9855
Conversation
JaroslavTulach
added
the
CI: No changelog needed
JaroslavTulach
requested review from
mwu-tow,
farmaazon,
kazcw,
vitvakatu and
Frizi
as code owners
| @@ -675,6 +675,16 @@ fn code_block_bad_indents1() { | |||
| test(&code.join("\n"), expected); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn fail_on_bad_indents() { | |||
| let code = ["group =", " space4", " space8", " space5"]; | |||
There was a problem hiding this comment.
The code is:
group =
space4
space8
space5e.g. block with indentation 4, followed by nested block with indentation 8 and then block with indentation 5 - a typical sign of error.
Right now the test succeeds - the goal is to make it fail!
There was a problem hiding this comment.
With the 350c2c3 change I am getting a failure in following tests:
code_block_bad_indents1
code_block_bad_indents2
fail_on_bad_indents
all the tests seem to resemble to problem reported in #9419 (comment) - e.g. it seems good that they are now failing. However I am not sure if the failure is the right one? I tried to insert Variant::invalid() - is that the right way to mark an indentation problem, @kazcw? Thanks in advance for your guidance.
There was a problem hiding this comment.
Expecting invalid node since 3e8e6c0
There was a problem hiding this comment.
token::Variant::invalid() is for an illegal token (e.g. an interpolation-quote outside of a text literal)--in this case the error it produces wouldn't be associated with the right span (which should be the rest of the tokens in the line). The marker token will also need to be located at the end of the preceding newline, not the beginning--token code references must cover the source code in order.
If this indentation-handling change is to be made, it could be done like this:
- In the lexer, construct a token of a new type
InvalidRestOfLine { error: Cow<'static, str> }. Use themarker_tokenfunction to attach the correct location information for a zero-length marker. - In the parser, we'll use a macro to handle it. The macro resolver isn't token-type aware, so we'll need to add the ability to use a zero-length marker token of a specific type as a macro segment header.
- Define a macro that matches the header
<InvalidRestOfLine>(this is the string we'll use to name the marker token; it would never match any real single token), followed byeverything(). Its implementation parses its tokens and wraps them in anInvalid1. - In the macro resolver, in
process_token, before looking up the token code in any macro maps, check if it is zero-length; if it is, match against the token's variant, and if it isInvalidRestOfLinelook up<InvalidRestOfLine>instead of the actual (empty) token code.
- Define a macro that matches the header
Let me know if you have any questions!
Footnotes
-
In the macro implementation, one tricky case will be if
precedence.resolvereturnsNone. This shouldn't be reachable now, but it might be possible with future uses ofInvalidRestOfLine, or in case of a bug (like the one I noted in another comment). The difficulty is that aTreemust still be created, and its spans (though 0-length) must still be appropriately-positioned within the document. You can handle this by applyinginto_identto the header token, and putting the result in a dummyTree::Identnode, to have something to wrap withInvalid. ↩
JaroslavTulach
changed the title
|
We have few failures:
|
JaroslavTulach
requested review from
jdunkerley,
GregoryTravis and
AdRiley
as code owners
| var errors = ir.preorder().filter((node) -> node instanceof Syntax); | ||
| assertEquals(1, errors.size()); | ||
| var first = (Syntax) errors.head(); | ||
| assertEquals(Syntax.UnexpectedExpression$.MODULE$, first.reason()); |
There was a problem hiding this comment.
What is the error message?
Is it possible to have the message mention 'invalid indentation level'? That would be much more helpful than just 'unexpected expression'.
There was a problem hiding this comment.
I guess I need to get that information from the Rust side. How do you want me to do it, @kazcw?
| @@ -1397,6 +1397,9 @@ impl<'s> Lexer<'s> { | |||
| // The new line indent is smaller than current block but bigger than the | |||
| // previous one. We are treating the line as belonging to the | |||
| // block. The warning should be reported by parser. | |||
| let location = newline.left_offset.code.position_before(); | |||
| let offset = Offset(VisibleOffset(0), location.clone()); | |||
| self.submit_token(Token(offset, location, token::Variant::invalid())); | |||
There was a problem hiding this comment.
When end_blocks is called when the input is at EOF, an error should not be emitted for a partial-indent in a whitespace-only final line. This test should pass:
fn partial_indent_at_eof_is_not_an_error() {
test("main =\n 42\n ", block![(Function /* ... */)]);
}| @@ -675,6 +675,16 @@ fn code_block_bad_indents1() { | |||
| test(&code.join("\n"), expected); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn fail_on_bad_indents() { | |||
| let code = ["group =", " space4", " space8", " space5"]; | |||
There was a problem hiding this comment.
token::Variant::invalid() is for an illegal token (e.g. an interpolation-quote outside of a text literal)--in this case the error it produces wouldn't be associated with the right span (which should be the rest of the tokens in the line). The marker token will also need to be located at the end of the preceding newline, not the beginning--token code references must cover the source code in order.
If this indentation-handling change is to be made, it could be done like this:
- In the lexer, construct a token of a new type
InvalidRestOfLine { error: Cow<'static, str> }. Use themarker_tokenfunction to attach the correct location information for a zero-length marker. - In the parser, we'll use a macro to handle it. The macro resolver isn't token-type aware, so we'll need to add the ability to use a zero-length marker token of a specific type as a macro segment header.
- Define a macro that matches the header
<InvalidRestOfLine>(this is the string we'll use to name the marker token; it would never match any real single token), followed byeverything(). Its implementation parses its tokens and wraps them in anInvalid1. - In the macro resolver, in
process_token, before looking up the token code in any macro maps, check if it is zero-length; if it is, match against the token's variant, and if it isInvalidRestOfLinelook up<InvalidRestOfLine>instead of the actual (empty) token code.
- Define a macro that matches the header
Let me know if you have any questions!
Footnotes
-
In the macro implementation, one tricky case will be if
precedence.resolvereturnsNone. This shouldn't be reachable now, but it might be possible with future uses ofInvalidRestOfLine, or in case of a bug (like the one I noted in another comment). The difficulty is that aTreemust still be created, and its spans (though 0-length) must still be appropriately-positioned within the document. You can handle this by applyinginto_identto the header token, and putting the result in a dummyTree::Identnode, to have something to wrap withInvalid. ↩
hubertp
added
the
CI: Clean build required
|
Closing stalled PR. |
Pull Request Description
Modify the parser to report an error on wrong indentation fixes #9419.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
changes, a screencast is preferred.