-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add Source Code Typing Tool #15211
base: master
Are you sure you want to change the base?
Add Source Code Typing Tool #15211
Conversation
spec/compiler/apply_types_spec.cr
Outdated
OUTPUT | ||
end | ||
|
||
# it "parses, runs semantic, and types everything" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting out this spec for now - it runs prelude as well as try to type everything the test specific file (but that test file isn't part of this PR yet).
# logic is in `crystal/tools/formatter.cr`. | ||
|
||
class Crystal::Command | ||
private def typer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entrypoint for the apply-types tool
@@ -0,0 +1,526 @@ | |||
module Crystal | |||
class SourceTyper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the source-typer repo, a lot of these classes / structs are broken into their own files. Precedence in the compiler seems to be to put a lot of these smaller bits into a single file if they can all be more or less self contained.
This pull request has been mentioned on Crystal Forum. There might be relevant details there: https://forum.crystal-lang.org/t/exploring-the-compiler/7343/21 |
This is awesome! |
@@ -39,6 +39,7 @@ class Crystal::Command | |||
Usage: crystal tool [tool] [switches] [program file] [--] [arguments] | |||
|
|||
Tool: | |||
apply-types add type restrictions to all untyped defs and def arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name suggestions welcome! Another option I thought of is apply-restrictions
as that feels more accurate, but also longer to type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bake-types
may also work, apply-restrictions
is also pretty good
This pull request has been mentioned on Crystal Forum. There might be relevant details there: https://forum.crystal-lang.org/t/rfc-variable-declaration-syntax-using/7623/35 |
Implement typer writing (inefficiently) Implement formatter, support multiple filename inputs Partial support for types, still need modules Update to latest source typer changes Add code comments Comment out broken spec for now Uncomment and fix final spec, add semantic / progress_tracker flags Remove focus: true (oops) Good ol' print debugging for windows CI failure Reimplement def visitor def locator matching for windows Back to print debugging Fix and support windows drive letters for root folders
…age when it happens
@@ -0,0 +1,580 @@ | |||
require "./spec_helper" | |||
|
|||
def run_source_typer_spec(input, expected_output, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specs are written with a before (INPUT) -> after (OUTPUT) pattern, which should hopefully showcase all of the use cases covered by the tool. All of the specs but the last one skip the prelude
run for performance.
rets = {} of String => String | ||
|
||
@warnings.each do |warning| | ||
puts "WARNING: #{warning}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warnings are cases where type restrictions weren't added because it crossed a line with the language syntax somehow (i.e. empty splat calls)
# And now infer types of everything | ||
semantic_node = program.semantic nodes, cleanup: true | ||
|
||
# We might run semantic later in an attempt to resolve defaults, don't display those stats or progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per this comment, a method can be defined like:
def my_method(arg = "str")
In this case, semantic may run on the "str"
(or whatever expression) in an attempt to figure out what it is. It's occasional that methods like these always get called with an argument that isn't the same type as the default, and then the compiler complains if the restriction ends up like:
def my_method(arg : Int32 = "str")
ret | ||
end | ||
|
||
private def def_overrides_parent_def(type) : Hash(String, String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method scans all defs
in type
and figures out the locations of all methods that override / are overridden between type
and its ancestor's definitions. These methods should not have their types restricted in any additional way. This method is required as there's no syntatic way to indicate one method overrides another.
end | ||
|
||
# Generates a map of (parsed) Def#location => Signature for that Def | ||
private def init_signatures(accepted_defs : Hash(String, Crystal::Def)) : Hash(String, Signature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workhorse method that generates the hash of location strings => signature for the def at that location
end | ||
end | ||
|
||
def flatten_types(types : Array(Crystal::Type)) : Array(Crystal::Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used to flatten unions into a single array, so dupe types can be deduplicated
end | ||
|
||
def type_name(type : Crystal::Type) : String | ||
type.to_s.gsub(/:Module$/, ".class").gsub("+", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an interesting discovery: metatypes of classes can be created using the .class
, but the syntax for creating a metatype of a module (also .class
) isn't the same string representation that the compiler uses. The more you know
# if there's a signature for a given def and those type restrictions are missing. | ||
# | ||
# All methods present are copy / paste from the original Crystal::Formatter for the given `visit` methods | ||
class SourceTyperFormatter < Crystal::Formatter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done typing, onto formatting!
# A visitor for defs, oddly enough. | ||
# | ||
# Walk through the AST and capture all references to Defs that match a def_locator | ||
class DefVisitor < Crystal::Visitor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefVisitor, a crystal visitor for.... visiting defs. Used for capturing a list of all passed through defs that match a def_locator used when running the CLI command.
This PR introduces a new
crystal tool apply-types
subcommand as described in this crystal forum post and initially implemented in this other repo as a stand alone tool. A tool that fully types a crystal file is obviously very tightly coupled with the language and compiler semantic, and so having this tool be part of the compiler itself feels like the correct direction.That being said, I'm not 100% it's ready yet, but I do think it's ready enough at least to solicit feedback from anyone who is willing to provide it :)