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.

Sign up for GitHub

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

Multiple highlighting improvements #506

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Multiple highlighting improvements #506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 77 additions & 17 deletions syntaxes/GDScript.tmLanguage.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,25 +132,51 @@
"end": "'''"
},
{
"begin": "\"",
"begin": "(r)?\"\"\"",
"end": "\"\"\"",
"patterns": [
{
"name": "constant.character.escape.untitled",
"match": "\\\\."
}
],
"beginCaptures": {
"1": {
"name": "constant.character.escape.untitled"
}
},
"name": "string.quoted.triple.gdscript"
},
{
"begin": "(r)?\"",
"end": "\"",
"patterns": [
{
"name": "constant.character.escape.untitled",
"match": "\\\\."
}
],
"beginCaptures": {
"1": {
"name": "constant.character.escape.untitled"
}
},
"name": "string.quoted.double.gdscript"
},
{
"begin": "'",
"begin": "(r)?'",
"end": "'",
"patterns": [
{
"name": "constant.character.escape.untitled",
"match": "\\\\."
}
],
"beginCaptures": {
"1": {
"name": "constant.character.escape.untitled"
}
},
"name": "string.quoted.single.gdscript"
},
{
Expand Down Expand Up @@ -309,37 +335,41 @@
"name": "keyword.operator.assignment.gdscript"
},
"control_flow": {
"match": "\\b(?i:if|elif|else|for|while|break|continue|pass|return|match|yield|await)\\b",
"match": "\\b(?:if|elif|else|for|while|break|continue|pass|return|match|yield|await)\\b",
"name": "keyword.control.gdscript"
},
"keywords": {
"match": "\\b(?i:class|class_name|extends|is|onready|tool|static|export|as|void|enum|preload|assert|breakpoint|rpc|sync|remote|master|puppet|slave|remotesync|mastersync|puppetsync|trait|namespace)\\b",
"match": "\\b(?:class|class_name|extends|is|onready|tool|static|export|as|void|enum|preload|assert|breakpoint|rpc|sync|remote|master|puppet|slave|remotesync|mastersync|puppetsync|trait|namespace)\\b",
"name": "keyword.language.gdscript"
},
"letter": {
"match": "\\b(?i:true|false|null)\\b",
"match": "\\b(?:true|false|null)\\b",
"name": "constant.language.gdscript"
},
"numbers": {
"patterns": [
{
"match": "\\b(?i:0x\\h*)\\b",
"match": "\\b(?:0b[01_]+)\\b",
"name": "constant.numeric.integer.binary.gdscript"
},
{
"match": "\\b(?:0x[0-9A-Fa-f_]+)\\b",
"name": "constant.numeric.integer.hexadecimal.gdscript"
},
{
"match": "\\b(?i:(\\d+\\.\\d*(e[\\-\\+]?\\d+)?))\\b",
"match": "\\b(?:([0-9_]+\\.[0-9_]*(e[\\-\\+]?[0-9_]+)?))\\b",
"name": "constant.numeric.float.gdscript"
},
{
"match": "\\b(?i:(\\.\\d+(e[\\-\\+]?\\d+)?))\\b",
"match": "\\b(?:(\\.[0-9_]+(e[\\-\\+]?[0-9_]+)?))\\b",
"name": "constant.numeric.float.gdscript"
},
{
"match": "\\b(?i:(\\d+e[\\-\\+]?\\d+))\\b",
"match": "\\b(?:([0-9_]+e[\\-\\+]?\\[0-9_]))\\b",
"name": "constant.numeric.float.gdscript"
},
{
"match": "\\b\\d+\\b",
"match": "\\b[0-9_]+\\b",
Copy link

Choose a reason for hiding this comment

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

It's not right to allow _ to intermix with numbers unconditionally. Cases like _0 are valid variable identifiers, and must not be colored like number literals. This regex need to be carefully thought out and tested, because adding the _ separator feature is not that simple. You need to ensure:

  • _ cannot appear at the beginning, or right after e in floats or x in hex literals
  • _ should not appear at the end, but it seems Godot allows it; better confirm this properly to know how to go about this
  • consecutive __ is not allowed

Copy link

@AlfishSoftware AlfishSoftware Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

On this particular line it would probably be enough to use "\\b\\d[0-9_]*\\b" if you won't forbid trailing _. On float and hex literals it'd probably be similar.

EDIT: Nevermind, forgot about the consecutive __. Yeah, this needs to be thought out properly.

Copy link
Collaborator Author

@DaelonSuzuka DaelonSuzuka Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not interested in completely validating numeric literals with the grammar. Invalid cases should get marked as syntax errors by the LSP.

Copy link

@AlfishSoftware AlfishSoftware Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's fair for invalid cases that aren't valid for other rules with less priority. But I believe valid variable names like _0, _0e0, _0x0 will be taken over by this rule and be incorrectly colored as numbers. At the bare minimum, these regexes gotta exclude the initial _ by using something like \d[0-9_]* instead of [0-9_]+.

But if you don't mind me completely validating it, the ideal would be:
int: "\\b\\d+(?:_\\d+)*\\b"
hex: "\\b0x[0-9A-Fa-f]+(?:_[0-9A-Fa-f]+)*\\b"
bin: "\\b0b[01]+(?:_[01]+)*\\b"
float1: "\\b\\d+(?:_\\d+)*\\.(?:\\d+(?:_\\d+)*)?(?:[eE][-+]?\\d+(?:_\\d+)*)?\\b"
float2: "\\.\\d+(?:_\\d+)*(?:[eE][-+]?\\d+(?:_\\d+)*)?\\b"
float3: "\\b\\d+(?:_\\d+)*[eE][-+]?\\d+(?:_\\d+)*\\b"

This should also fix:

  • [eE] exponent which can be uppercase too
  • a typo where the \\ part of \\d+ on line 338->368 wasn't removed
  • float2 starts with . (non-word character) so it must not be preceded by a \b word boundary

Sorry for writing this in a comment instead of making a PR or something.

Copy link
Collaborator Author

@DaelonSuzuka DaelonSuzuka Oct 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

... other rules with less priority. But I believe valid variable names like _0, _0e0, _0x0 will be taken over by this rule ...

Ah, I'm with you now; I misunderstood the problem.

I'll add some new test cases and then pop in your rules. Thanks for writing them up.

Sorry for writing this in a comment instead of making a PR or something.

No worries at all. Your second set of eyes has always been a big help.

"name": "constant.numeric.integer.gdscript"
}
]
Expand Down Expand Up @@ -494,6 +524,9 @@
},
"builtin_get_node_shorthand": {
"patterns": [
{
"include": "#builtin_get_nodepath_shorthand_quoted"
},
{
"include": "#builtin_get_node_shorthand_quoted"
},
Expand All @@ -502,6 +535,30 @@
}
]
},
"builtin_get_nodepath_shorthand_quoted": {
"begin": "(&|\\^|@)([\"'])",
"end": "([\"'])",
"name": "support.function.builtin.shorthand.gdscript",
"beginCaptures": {
"1": {
"name": "variable.other.enummember"
},
"2": {
"name": "constant.character.escape"
}
},
"endCaptures": {
"1": {
"name": "constant.character.escape"
}
},
"patterns": [
{
"match": "[^%^\"^']*",
"name": "constant.character.escape"
}
]
},
"builtin_get_node_shorthand_quoted": {
"begin": "(\\$)([\"'])",
Copy link

Choose a reason for hiding this comment

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

Godot 4 also accepts %"..." and %'...' quoted syntax.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch.

"end": "([\"'])",
Expand Down Expand Up @@ -531,7 +588,7 @@
]
},
"builtin_get_node_shorthand_bare": {
"begin": "(\\$)",
"begin": "(\\$|%)",
"end": "[^\\w%]",
"name": "support.function.builtin.shorthand.gdscript",
"beginCaptures": {
Expand All @@ -541,12 +598,15 @@
},
"patterns": [
{
"match": "[a-zA-Z_]\\w*/?",
"name": "constant.character.escape"
},
{
"match": "%[a-zA-Z_]\\w*/?",
"name": "invalid.illegal.escape.gdscript"
"match": "(%)?([a-zA-Z_]\\w*/?)",
"captures": {
"1": {
"name": "keyword.control.flow"
},
"2": {
"name": "constant.character.escape"
}
}
}
]
},
Expand Down
27 changes: 20 additions & 7 deletions syntaxes/examples/gdscript1.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@ var var_f:bool=true
var var_g : string = 'foo'
var var_h : string = "foo"

var integer = 12_345_678 # Equal to 12345678.
var floating = 3.141_592_7 # Equal to 3.1415927.
var hex_a = 0x10101
var hex_b = 0x8080_0000_ffff # Equal to 0x80800000ffff.
var binary_a = 0b10101
var binary_b = 0b11_00_11_00 # Equal to 0b11001100.

var sci_float_a = 58.1e-10
var sci_float_b = 58.1e-10

const const_a = 0
const const_b = true
const const_c := true
Expand Down Expand Up @@ -126,6 +136,9 @@ func one_line_fn() -> void: return

# ------------------------------------------------------------------------------

var stringname_nodepath_a = @"test"
var stringname_nodepath_b = @'test'

var q = "double quotes"
var r = 'single quotes'
var s = """
Expand Down Expand Up @@ -158,13 +171,13 @@ onready var node_h = get_node("../Sibling")
if has_node('Child') and get_node('Child').has_node('GrandChild'):
pass

#! NOTE: scene unique nodes can only appear inside quoted nodepaths, not
#! naked ones using the $ operator

onready var bad_unique_nodepath_a = $%Unique
onready var bad_unique_nodepath_b = $Child/%Unique
onready var bad_unique_nodepath_c = $Child/GrandChild/%Unique
onready var bad_unique_nodepath_c = $Child/%Unique/ChildOfUnique
onready var unique_node_a = $%Unique
onready var unique_node_b = $Child/%Unique
onready var unique_node_c = $Child/GrandChild/%Unique
onready var unique_node_d = $Child/%Unique/ChildOfUnique
onready var unique_node_e = %Unique
onready var unique_node_f = %Unique/Child
onready var unique_node_g = %Unique/%UniqueChild

onready var node_i = $"%Unique"
onready var node_ii = get_node("%Unique")
Expand Down
9 changes: 9 additions & 0 deletions syntaxes/examples/gdscript2.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@ class_name TestClass2
@export var z : String
@export_node_path(Resource) var resource_name

var raw_string_a = r"test"
var raw_string_b = r'test'
var raw_string_c = r"""test"""

var nodepath_a = &"test"
var nodepath_b = &'test'
var stringname_a = ^"test"
var stringname_b = ^'test'

var array_a: Array[int] = [1, 2, 3]
var array_b: Array[String] = ['1', '2', '3']

Expand Down