Skip to content

Highlight all soft modifiers #168

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 10 additions & 14 deletions src/typescript/Scala.tmLanguage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -584,21 +584,13 @@ export const scalaTmLanguage: TmLanguage = {
},
inline: {
patterns: [
{
match: `\\b(inline)\\s+(?=(([\\w\\s]*\\b(val|def|given)\\b)|(${plainid}|${backQuotedId})\\s*:))`,
captures: {
'1': {
name: 'storage.modifier.other'
}
}
{ // inline parameters
match: `\\b(inline)(?=\\s+(${plainid}|${backQuotedId})\\s*:)`,
name: 'storage.modifier.other'
},
{
match: `\\b(inline)\\b(?=(?:.(?!\\b(?:val|def|given)\\b))*\\b(if|match)\\b)`,
captures: {
'1': {
name: 'keyword.control.flow.scala'
}
}
name: 'keyword.control.flow.scala'
}
]
},
Expand Down Expand Up @@ -651,7 +643,7 @@ export const scalaTmLanguage: TmLanguage = {
}
},
{
match: `\\b(?:(case|open)\\s+)?(class|object|enum)\\s+([^\\s\\{\\(\\[;]+)(?<![^${opchar}]:)`,
match: `\\b(?:(case)\\s+)?(class|object|enum)\\s+([^\\s\\{\\(\\[;]+)(?<![^${opchar}]:)`,
captures: {
'1': {
name: 'keyword.declaration.scala'
Expand Down Expand Up @@ -846,7 +838,11 @@ export const scalaTmLanguage: TmLanguage = {
name: 'storage.modifier.access'
},
{
match: '\\b(synchronized|@volatile|abstract|final|lazy|sealed|implicit|opaque |override|@transient|@native)\\b',
match: '\\b(synchronized|@volatile|abstract|final|lazy|sealed|implicit|override|@transient|@native)\\b',
name: 'storage.modifier.other'
},
{
match: '(?<=^|\\s)\\b(transparent|opaque|infix|open|inline)\\b(?=[a-z\\s]*\\b(def|val|var|given|type|class|trait|object|enum)\\b)',
Copy link
Contributor

@MaximeKjaer MaximeKjaer Dec 10, 2020

Choose a reason for hiding this comment

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

Instead of allowing [a-z\\s]* between a soft keyword modifier and a keyword, I think we should aim to only highlight a chain of soft keywords followed by a keyword, something like ((transparent|opaque|infix|open|inline)\\b)+(?=\\b(def|val|var|given|type|class|trait|object|enum)) (might need some adjustment for allowing whitespace). With soft keywords, I think we should try to be as minimal as possible, only highlighting them as keywords if strictly necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that a soft modifier can be followed by a sequence of normal modifiers. [a-z\\s]* skips those modifiers to find the def|val|....

Copy link
Contributor

Choose a reason for hiding this comment

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

The alternative would be to list all modifiers separated by spaces instead of [a-z\\s]*

name: 'storage.modifier.other'
}
]
Expand Down
71 changes: 71 additions & 0 deletions tests/unit/#155.test.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// SYNTAX TEST "source.scala"

transparent inline def foo
// ^^^^^^^^^^^ storage.modifier.other
// ^^^^^^ storage.modifier.other

transparent inline xdef foo
// ^^^^^^^^^^^ - storage.modifier.other
// ^^^^^^ - storage.modifier.other

transparent inline defx foo
// ^^^^^^^^^^^ - storage.modifier.other
// ^^^^^^ - storage.modifier.other

transparent inline final def assert
// ^^^^^^^^^^^ storage.modifier.other
// ^^^^^^ storage.modifier.other

transparent badkeyword inline override def alternative
// ^^^^^^^^^^^ storage.modifier.other
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to the previous comment: I don't think that we should have a test requiring that transparent is storage.modifier.other in this situation. The general rule of thumb that I've been applying is that when it's invalid Scala, it can be highlighted as anything; we shouldn't require specific highlightings for invalid Scala in our tests, because that could make future refactorings more difficult.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a consequence of the [a-z\\s]* above.

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be practical if we have

transparent inilne def alternative

In this case transparent and def are highlighted but not inilne which makes it trivial to find the typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that's actually a quite convincing argument for keeping [a-z\\s]*. In that case, we can merge the PR.

// ^^^^^^ storage.modifier.other

transparent trait Enum extends Any with Product with Serializable
// ^^^^^^^^^^^ storage.modifier.other

opaque type Logarithm = Double
// ^^^^^^ storage.modifier.other

opaque private type Matching = Option[Tuple]
// ^^^^^^ storage.modifier.other

infix def + (that: Rational) =
// ^^^^^ storage.modifier.other

infix type +[X <: Int | String, Y <: Int | String] = (X, Y) match
// ^^^^^ storage.modifier.other

open class Open
// ^^^^ storage.modifier.other

open final class Foo1
// ^^^^ storage.modifier.other

open final class Foo1
// <---- storage.modifier.other

@inline def
// ^^^^^^ - storage.modifier.other

@infix def
// ^^^^^ - storage.modifier.other

@transparent def
// ^^^^^^^^^^^ - storage.modifier.other

@opaque def
// ^^^^^^ - storage.modifier.other

@open def
// ^^^^ - storage.modifier.other

@scala.inline def
// ^^^^^^ - storage.modifier.other

@infix inline def
// ^^^^^ - storage.modifier.other
// ^^^^^^ storage.modifier.other

file.open()
// ^^^^ - storage.modifier.other

2 changes: 1 addition & 1 deletion tests/unit/#91.test.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// SYNTAX TEST "source.scala"

open class A
// ^^^^ keyword.declaration.scala
// ^^^^ storage.modifier.other
// ^^^^^ keyword.declaration.scala
// ^ entity.name.class.declaration