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

[3.x] Fix inner class parsing when statement after colon is on the same line (reverted) #61993

Merged

Conversation

kdiduk
Copy link
Contributor

@kdiduk kdiduk commented Jun 13, 2022

This commit fixes #56703

The fix is to increase indent level when the statement after the colon token is on the same line.

It looks like this issue is a regression introduced after some of the changes. Because in one of the earlier versions of that piece of code the intend level was increased (see this, for example).

Bugsquad edit:

@kdiduk kdiduk requested a review from a team as a code owner June 13, 2022 12:50
@Chaosus Chaosus added this to the 3.x milestone Jun 13, 2022
@kdiduk
Copy link
Contributor Author

kdiduk commented Jun 13, 2022

If this PR is approved and merged, we need to update documentation for GDScript grammar (both in 3.x and 4.0), because it says that after the colon in the inner class declaration there is a newline:

innerClass = "class" IDENTIFIER [ inheritance ] ":" NEWLINE
    INDENT [ inheritance NEWLINE ] topLevelDecl { topLevelDecl } DEDENT ;

As I understand, the idea is to have it more Python'ic way, where this is allowed:

class Foo: pass

At least in 4.0 it already works like that.

@akien-mga
Copy link
Member

It looks like this issue is a regression introduced after some of the changes. Because in one of the earlier versions of that piece of code the intend level was increased (see this, for example).

For the record, the +1 was removed in this PR: #8123.
And that code was further refactored in #32808.

Please make sure that this doesn't reintroduce the issues that those PRs fixed.

@kdiduk kdiduk changed the title Fix inner class parsing when statement after colos in on the same line Fix inner class parsing when statement after colos is on the same line Jun 13, 2022
@kdiduk kdiduk force-pushed the gdscript-fix-inner-class-on-single-line branch from b77e3ef to 942b304 Compare June 16, 2022 05:57
@kdiduk
Copy link
Contributor Author

kdiduk commented Jun 16, 2022

Please make sure that this doesn't reintroduce the issues that those PRs fixed.

@akien-mga Thank you for mentioning the original PR! Indeed my initial fix re-introduced some issues from them.

I've just pushed another implementation. I've implemented a special case for inner classes and it doesn't touch the cases mentioned in those PRs.

In this implementation, it's allowed to write like this:

class Foo: pass

And also, as in Python, it's not allowed to use anything else apart from pass in one-liner class, like that:

class Foo: func bar(): pass

Some of the test-cases that I've used can be found here:

test_inner_classes.gd
extends Node2D

class Foo: 
	func foo():
		return "Hello, Foo!"

class Bar extends Foo: pass # inner class

# not allowed: says "unexpected indentation"
# class BadBar: pass
# 	func bad_bar():
# 		pass

class FooBar extends Bar:
	func foo():
		return "Hello, FooBar!"

var foo_var: FooBar = FooBar.new()

class AnotherInnerClass:
	func foo(): pass


func _ready(): $RichTextLabel.text = foo_var.foo()

# at the end of file
class BarFoo extends Foo: pass

I can also do some refactoring to clean up and eliminate duplicated code.

@kdiduk kdiduk force-pushed the gdscript-fix-inner-class-on-single-line branch from 942b304 to f1ce18a Compare June 16, 2022 06:15
@kdiduk kdiduk changed the title Fix inner class parsing when statement after colos is on the same line Fix inner class parsing when statement after colon is on the same line Jun 16, 2022
modules/gdscript/gdscript_parser.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.cpp Outdated Show resolved Hide resolved
@vnen
Copy link
Member

vnen commented Jun 17, 2022

Looks good to me, my remarks are only about the comment formatting.

@kdiduk kdiduk force-pushed the gdscript-fix-inner-class-on-single-line branch from f1ce18a to 741efb2 Compare June 17, 2022 21:57
@kdiduk
Copy link
Contributor Author

kdiduk commented Jun 17, 2022

@vnen Thank you for your remarks. I've just pushed new changes with some refactoring. I have eliminated duplicated code blocks, extracting it into separate methods. Could you review it again?

@kdiduk kdiduk requested a review from vnen June 17, 2022 22:04
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

Looks good to me, just have one style remark.

modules/gdscript/gdscript_parser.cpp Outdated Show resolved Hide resolved
…e line

Implement a special case for allowing "pass" keyword in one-liner class
declaration, to be consistent with Python style.

```
class TestClass: pass
```

This commit fixes godotengine#56703
@kdiduk kdiduk force-pushed the gdscript-fix-inner-class-on-single-line branch from f830f46 to 5bcc3d4 Compare June 21, 2022 08:04
@kdiduk kdiduk requested a review from vnen June 21, 2022 08:05
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

Looks good to me

@kdiduk
Copy link
Contributor Author

kdiduk commented Jun 28, 2022

FYI: this pull-request is in the milestone 3.x, although the bug that it fixes #56703 is in the milestone 3.5. I guess the both should be in the same milestone.

@akien-mga akien-mga added the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Jun 28, 2022
@akien-mga akien-mga merged commit 5197dc6 into godotengine:3.x Aug 5, 2022
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 3.x, 3.6 Aug 5, 2022
@kdiduk kdiduk deleted the gdscript-fix-inner-class-on-single-line branch August 8, 2022 09:00
@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Aug 30, 2022
@akien-mga akien-mga changed the title Fix inner class parsing when statement after colon is on the same line Fix inner class parsing when statement after colon is on the same line (reverted) Jul 17, 2024
@akien-mga akien-mga changed the title Fix inner class parsing when statement after colon is on the same line (reverted) [3.x] Fix inner class parsing when statement after colon is on the same line (reverted) Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants