From 64db1fd70130d962a4b986e238f0471bd033f0c2 Mon Sep 17 00:00:00 2001 From: Alex Pinkus Date: Sat, 6 Nov 2021 12:04:16 -0700 Subject: [PATCH] Add restrictions on modifiers in local scope Declarations for functions, classes, and variables are legal at various different scopes (global, type-level, and local), but have different sets of legal modifiers in each. While you might think that would be fine and would simply improve our permissiveness, it actually ends up _restricting_ some valid code - in particular, code that uses those would-be modifiers as identifiers. For instance, one file in Carthage uses the innocent-looking variable name `prefix`, but we fail to parse that because in other scopes, that would instead be a modifier. To fix this, we split out declarations by scope: global, type-level, and local. For now, we treat global and type-level as mostly the same except that subscripts are legal in the latter and not the former. However, for local declarations, we allow a much smaller list of operators. Fixes #39 --- corpus/classes.txt | 71 +++++++------- corpus/expressions.txt | 53 +++++++---- corpus/functions.txt | 4 +- corpus/statements.txt | 93 ++++++++++++++++--- grammar.js | 163 ++++++++++++++++++++++++++------- script-data/known_failures.txt | 2 - 6 files changed, 284 insertions(+), 102 deletions(-) diff --git a/corpus/classes.txt b/corpus/classes.txt index 67fce06..dfdbf42 100755 --- a/corpus/classes.txt +++ b/corpus/classes.txt @@ -84,7 +84,7 @@ internal open class Test { (source_file (class_declaration - (modifiers (visibility_modifier) (inheritance_modifier)) + (modifiers (visibility_modifier) (visibility_modifier)) (type_identifier) (class_body (property_declaration @@ -159,7 +159,7 @@ class GenericSubscript { (type_identifier) (class_body (subscript_declaration - (modifiers (inheritance_modifier)) + (modifiers (visibility_modifier)) (parameter (simple_identifier) (user_type (type_identifier))) (optional_type (user_type (type_identifier))) (computed_getter @@ -201,40 +201,45 @@ class GenericSubscript { Subscript with multiple arguments === -public subscript(_ value: D, arg2: Arg2) -> D where D: Decodable { - return value +class Subscriptable { + public subscript(_ value: D, arg2: Arg2) -> D where D: Decodable { + return value + } } --- -(source_file - (subscript_declaration - (modifiers - (visibility_modifier)) - (type_parameters - (type_parameter - (type_identifier))) - (external_parameter_name) - (parameter - (simple_identifier) - (user_type - (type_identifier))) - (parameter - (simple_identifier) - (user_type - (type_identifier))) - (user_type - (type_identifier)) - (type_constraints - (type_constraint - (inheritance_constraint - (identifier - (simple_identifier)) - (user_type - (type_identifier))))) - (statements - (control_transfer_statement - (simple_identifier))))) + (source_file + (class_declaration + (type_identifier) + (class_body + (subscript_declaration + (modifiers + (visibility_modifier)) + (type_parameters + (type_parameter + (type_identifier))) + (external_parameter_name) + (parameter + (simple_identifier) + (user_type + (type_identifier))) + (parameter + (simple_identifier) + (user_type + (type_identifier))) + (user_type + (type_identifier)) + (type_constraints + (type_constraint + (inheritance_constraint + (identifier + (simple_identifier)) + (user_type + (type_identifier))))) + (statements + (control_transfer_statement + (simple_identifier))))))) ================== Inheritance @@ -303,7 +308,7 @@ class SomethingElse: ThingProvider { (type_annotation (optional_type (user_type (type_identifier))))) (property_declaration - (modifiers (property_modifier)) + (modifiers (property_behavior_modifier)) (value_binding_pattern (non_binding_pattern (simple_identifier))) (type_annotation (user_type (type_identifier))) diff --git a/corpus/expressions.txt b/corpus/expressions.txt index a231a00..0a40e73 100755 --- a/corpus/expressions.txt +++ b/corpus/expressions.txt @@ -110,15 +110,19 @@ someVeryLongFunctionCall() Multiple expressions on one line using semicolons ================== -print(a); return b +print(a); throw b --- -(source_file - (call_expression + (source_file + (call_expression (simple_identifier) - (call_suffix (value_arguments (value_argument (simple_identifier))))) - (control_transfer_statement (simple_identifier))) + (call_suffix + (value_arguments + (value_argument + (simple_identifier))))) + (throw_keyword) + (simple_identifier)) ================== Indexing @@ -230,20 +234,37 @@ let a: SomeClass = .someInstance Tuple expressions ================== -let a: (Int, Int) = (1, 2) -return (lhs: 3, rhs: 4) +func math() { + let a: (Int, Int) = (1, 2) + return (lhs: 3, rhs: 4) +} --- -(source_file - (property_declaration - (value_binding_pattern (non_binding_pattern (simple_identifier))) - (type_annotation (tuple_type (user_type (type_identifier)) (user_type (type_identifier)))) - (tuple_expression (integer_literal) (integer_literal))) - (control_transfer_statement - (tuple_expression - (simple_identifier) (integer_literal) - (simple_identifier) (integer_literal)))) + (source_file + (function_declaration + (simple_identifier) + (function_body + (statements + (property_declaration + (value_binding_pattern + (non_binding_pattern + (simple_identifier))) + (type_annotation + (tuple_type + (user_type + (type_identifier)) + (user_type + (type_identifier)))) + (tuple_expression + (integer_literal) + (integer_literal))) + (control_transfer_statement + (tuple_expression + (simple_identifier) + (integer_literal) + (simple_identifier) + (integer_literal))))))) ================== Function calls diff --git a/corpus/functions.txt b/corpus/functions.txt index b15214f..0e621b7 100755 --- a/corpus/functions.txt +++ b/corpus/functions.txt @@ -390,7 +390,7 @@ test { [weak self, otherSelf] (a) in } (simple_identifier) (call_suffix (lambda_literal - (capture_list (simple_identifier) (simple_identifier)) + (capture_list (ownership_modifier) (simple_identifier) (simple_identifier)) (lambda_function_type (lambda_function_type_parameters (simple_identifier))))))) ================== @@ -469,7 +469,7 @@ private lazy var onCatClosure: (_ cat: Cat) throws -> Void = { _ in (source_file (property_declaration - (modifiers (visibility_modifier) (property_modifier)) + (modifiers (visibility_modifier) (property_behavior_modifier)) (value_binding_pattern (non_binding_pattern (simple_identifier))) (type_annotation (function_type diff --git a/corpus/statements.txt b/corpus/statements.txt index cc5deb0..fd138d6 100755 --- a/corpus/statements.txt +++ b/corpus/statements.txt @@ -356,25 +356,39 @@ someLabel: if a.isEmpty, let b = getB() { If let with function call =============== -if let a = try? Foo.getValue("key") { - return a +func doSomething() { + if let a = try? Foo.getValue("key") { + return a + } + return default } -return default --- -(source_file - (if_statement - (value_binding_pattern (non_binding_pattern (simple_identifier))) - (call_expression - (try_expression - (navigation_expression (simple_identifier) - (navigation_suffix (simple_identifier)))) - (call_suffix - (value_arguments - (value_argument (line_string_literal))))) - (statements (control_transfer_statement (simple_identifier)))) - (control_transfer_statement (simple_identifier))) + (source_file + (function_declaration + (simple_identifier) + (function_body + (statements + (if_statement + (value_binding_pattern + (non_binding_pattern + (simple_identifier))) + (call_expression + (try_expression + (navigation_expression + (simple_identifier) + (navigation_suffix + (simple_identifier)))) + (call_suffix + (value_arguments + (value_argument + (line_string_literal))))) + (statements + (control_transfer_statement + (simple_identifier)))) + (control_transfer_statement + (simple_identifier)))))) ================== Compound if let @@ -531,3 +545,52 @@ let ΓΈ = unicode() (simple_identifier) (call_suffix (value_arguments))))) + +=== +Contextual keywords are usable as identifiers +=== + +public init() { + // `prefix` doesn't work as a function modifier here, so it's legal as an identifier + prefix = prefixToJSON + + // But some modifiers are legal! + weak var weakPrefix = prefix + final class LocalClass { } + + // Annotations are legal too! + @someAnnotation + func innerFunc() { } +} + +--- + +(source_file + (function_declaration + (modifiers + (visibility_modifier)) + (function_body + (comment) + (statements + (assignment + (directly_assignable_expression + (simple_identifier)) + (simple_identifier)) + (comment) + (property_declaration + (ownership_modifier) + (value_binding_pattern + (non_binding_pattern + (simple_identifier))) + (simple_identifier)) + (class_declaration + (inheritance_modifier) + (type_identifier) + (class_body)) + (comment) + (function_declaration + (attribute + (user_type + (type_identifier))) + (simple_identifier) + (function_body)))))) diff --git a/grammar.js b/grammar.js index 94ee036..b644aa2 100644 --- a/grammar.js +++ b/grammar.js @@ -89,6 +89,10 @@ module.exports = grammar({ // (start: start, end: end) [$._tuple_type_item_identifier, $.tuple_expression], + + // After a `{` in a function or switch context, it's ambigous whether we're starting a set of local statements or + // applying some modifiers to a capture or pattern. + [$.modifiers], ], extras: ($) => [ @@ -147,7 +151,10 @@ module.exports = grammar({ //////////////////////////////// source_file: ($) => - seq(optional($.shebang_line), repeat(seq($._statement, $._semi))), + seq( + optional($.shebang_line), + repeat(seq($._top_level_statement, $._semi)) + ), shebang_line: ($) => seq("#!", /[^\r\n]*/), @@ -634,16 +641,13 @@ module.exports = grammar({ choice( "self", seq( - optional($._capture_specifier), + optional($.ownership_modifier), $.simple_identifier, optional(seq($._equal_sign, $._expression)) ) ) ), - _capture_specifier: ($) => - choice("weak", "unowned", "unowned(safe)", "unowned(unsafe)"), - lambda_function_type: ($) => seq( choice( @@ -814,17 +818,29 @@ module.exports = grammar({ statements: ($) => prec.left( - seq($._statement, repeat(seq($._semi, $._statement)), optional($._semi)) + seq( + $._local_statement, + repeat(seq($._semi, $._local_statement)), + optional($._semi) + ) ), - _statement: ($) => + _local_statement: ($) => choice( $._expression, - $._declaration, + $._local_declaration, $._labeled_statement, $.control_transfer_statement, - $.assignment, - $.availability_condition + $.assignment + ), + + _top_level_statement: ($) => + choice( + $._expression, + $._global_declaration, + $._labeled_statement, + $._throw_statement, + $.assignment ), _block: ($) => prec(PREC.BLOCK, seq("{", optional($.statements), "}")), @@ -880,10 +896,13 @@ module.exports = grammar({ control_transfer_statement: ($) => choice( - prec.right(seq("throw", $._expression)), + prec.right($._throw_statement), prec.right(seq($._return_continue_break, optional($._expression))) ), + _throw_statement: ($) => seq($.throw_keyword, $._expression), + throw_keyword: ($) => "throw", + _return_continue_break: ($) => choice("return", "continue", "break"), assignment: ($) => @@ -911,7 +930,7 @@ module.exports = grammar({ // Declarations - https://docs.swift.org/swift-book/ReferenceManual/Declarations.html //////////////////////////////// - _declaration: ($) => + _global_declaration: ($) => choice( $.import_declaration, $.property_declaration, @@ -920,6 +939,19 @@ module.exports = grammar({ $.class_declaration, // TODO actor declaration $.protocol_declaration, + $.operator_declaration, + $.precedence_group_declaration, + $.associatedtype_declaration + ), + + _type_level_declaration: ($) => + choice( + $.import_declaration, + $.property_declaration, + $.typealias_declaration, + $.function_declaration, + $.class_declaration, + $.protocol_declaration, $.deinit_declaration, $.subscript_declaration, $.operator_declaration, @@ -927,6 +959,38 @@ module.exports = grammar({ $.associatedtype_declaration ), + _local_declaration: ($) => + choice( + alias($._local_property_declaration, $.property_declaration), + alias($._local_typealias_declaration, $.typealias_declaration), + alias($._local_function_declaration, $.function_declaration), + alias($._local_class_declaration, $.class_declaration) + ), + + _local_property_declaration: ($) => + seq( + optional($._locally_permitted_modifiers), + $._modifierless_property_declaration + ), + + _local_typealias_declaration: ($) => + seq( + optional($._locally_permitted_modifiers), + $._modifierless_typealias_declaration + ), + + _local_function_declaration: ($) => + seq( + optional($._locally_permitted_modifiers), + $._modifierless_function_declaration + ), + + _local_class_declaration: ($) => + seq( + optional($._locally_permitted_modifiers), + $._modifierless_class_declaration + ), + import_declaration: ($) => seq( optional($.modifiers), @@ -962,10 +1026,15 @@ module.exports = grammar({ seq("{", repeat(choice($.getter_specifier, $.setter_specifier)), "}"), property_declaration: ($) => + seq( + optional($.modifiers), + optional("class"), + $._modifierless_property_declaration + ), + + _modifierless_property_declaration: ($) => prec.right( seq( - optional($.modifiers), - optional("class"), choice("let", "var"), sep1( seq( @@ -985,8 +1054,10 @@ module.exports = grammar({ generate_pattern_matching_rule($, false, false), typealias_declaration: ($) => + seq(optional($.modifiers), $._modifierless_typealias_declaration), + + _modifierless_typealias_declaration: ($) => seq( - optional($.modifiers), "typealias", alias($.simple_identifier, $.type_identifier), optional($.type_parameters), @@ -997,11 +1068,21 @@ module.exports = grammar({ function_declaration: ($) => prec.right(seq($._bodyless_function_declaration, $.function_body)), + _modifierless_function_declaration: ($) => + prec.right( + seq($._modifierless_function_declaration_no_body, $.function_body) + ), + _bodyless_function_declaration: ($) => + seq( + optional($.modifiers), + optional("class"), // XXX: This should be possible in non-last position, but that creates parsing ambiguity + $._modifierless_function_declaration_no_body + ), + + _modifierless_function_declaration_no_body: ($) => prec.right( seq( - optional($.modifiers), - optional("class"), // XXX: This should be possible in non-last position, but that creates parsing ambiguity choice( $._constructor_function_decl, $._non_constructor_function_decl @@ -1019,10 +1100,12 @@ module.exports = grammar({ function_body: ($) => $._block, class_declaration: ($) => + seq(optional($.modifiers), $._modifierless_class_declaration), + + _modifierless_class_declaration: ($) => prec.right( choice( seq( - optional($.modifiers), choice("class", "struct"), alias($.simple_identifier, $.type_identifier), optional($.type_parameters), @@ -1031,7 +1114,6 @@ module.exports = grammar({ $.class_body ), seq( - optional($.modifiers), "extension", $.identifier, optional($.type_parameters), @@ -1040,7 +1122,6 @@ module.exports = grammar({ $.class_body ), seq( - optional($.modifiers), "enum", alias($.simple_identifier, $.type_identifier), optional($.type_parameters), @@ -1088,7 +1169,8 @@ module.exports = grammar({ equality_constraint: ($) => seq(repeat($.attribute), $.identifier, choice("=", "=="), $._type), - _class_member_declarations: ($) => repeat1(seq($._declaration, $._semi)), + _class_member_declarations: ($) => + repeat1(seq($._type_level_declaration, $._semi)), _function_value_parameters: ($) => seq("(", optional(sep1($._function_value_parameter, ",")), ")"), @@ -1135,7 +1217,7 @@ module.exports = grammar({ throws_modifier: ($) => choice($._throws_keyword, $._rethrows_keyword), enum_class_body: ($) => - seq("{", repeat(choice($.enum_entry, $._declaration)), "}"), + seq("{", repeat(choice($.enum_entry, $._type_level_declaration)), "}"), enum_entry: ($) => prec.left( @@ -1355,29 +1437,44 @@ module.exports = grammar({ // Modifiers // ========== - modifiers: ($) => repeat1(choice($.attribute, $._modifier)), + modifiers: ($) => + repeat1( + choice($._non_local_scope_modifier, $._locally_permitted_modifiers) + ), + _locally_permitted_modifiers: ($) => + repeat1(choice($.attribute, $._locally_permitted_modifier)), parameter_modifiers: ($) => repeat1($.parameter_modifier), _modifier: ($) => + choice($._non_local_scope_modifier, $._locally_permitted_modifier), + + _non_local_scope_modifier: ($) => choice( $.member_modifier, $.visibility_modifier, $.function_modifier, $.mutation_modifier, $.property_modifier, - $.inheritance_modifier, $.parameter_modifier ), + _locally_permitted_modifier: ($) => + choice( + $.ownership_modifier, + $.property_behavior_modifier, + $.inheritance_modifier + ), + + property_behavior_modifier: ($) => "lazy", + type_modifiers: ($) => repeat1($.attribute), - member_modifier: ($) => - choice("override", "convenience", "required", "unowned", "weak"), + member_modifier: ($) => choice("override", "convenience", "required"), visibility_modifier: ($) => seq( - choice("public", "private", "internal", "fileprivate"), + choice("public", "private", "internal", "fileprivate", "open"), optional(seq("(", "set", ")")) ), @@ -1387,17 +1484,15 @@ module.exports = grammar({ mutation_modifier: ($) => choice("mutating", "nonmutating"), - property_modifier: ($) => - choice( - "static", // XXX should be in multiple places - "lazy", - "optional" - ), + property_modifier: ($) => choice("static", "optional"), - inheritance_modifier: ($) => choice("final", "open"), + inheritance_modifier: ($) => choice("final"), parameter_modifier: ($) => choice("inout", "@escaping", "@autoclosure"), + ownership_modifier: ($) => + choice("weak", "unowned", "unowned(safe)", "unowned(unsafe)"), + use_site_target: ($) => seq( choice( diff --git a/script-data/known_failures.txt b/script-data/known_failures.txt index 2b7d1bf..f9b2ff0 100644 --- a/script-data/known_failures.txt +++ b/script-data/known_failures.txt @@ -7,12 +7,10 @@ Carthage/Source/carthage/Build.swift Carthage/Source/carthage/Checkout.swift Carthage/Source/carthage/Formatting.swift Carthage/Source/CarthageKit/Archive.swift -Carthage/Source/CarthageKit/VersionFile.swift Carthage/Source/CarthageKit/Project.swift Carthage/Source/CarthageKit/GitURL.swift Carthage/Source/CarthageKit/Git.swift Carthage/Tests/CarthageKitTests/DependencySpec.swift Carthage/Tests/CarthageKitTests/CartfileCommentsSpec.swift ObjectMapper/Package@swift-4.swift -ObjectMapper/Sources/HexColorTransform.swift ObjectMapper/Sources/ToJSON.swift