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

Support for built-in @type and @spec #113

Merged
merged 46 commits into from
May 31, 2022

Conversation

orsinium
Copy link
Contributor

@orsinium orsinium commented May 27, 2022

The complete version of #109

Adds the following public functions:

  1. fetch_spec to extract type from @spec of the given function.
  2. fetch_type to extract type from @type with the given name.
  3. apply which is the same as Kernel.apply but also checks types (both arguments and result).
  4. ensure_types macro which extracts types from @spec and puts it into apply.

The last one is experimental and isn't very stable. Areas for improvement:

  1. If it can't fetch the given spec, it will fail with a pattern matching error. It would be better to provide a friendlier error for such cases.
  2. I've created The full function name inference (ast_to_mfa) using only my own head and my own experiments in REPL. It might miss some corner cases, I'm not proficient with Elixir AST yet.

Other than that, it should be good enough. The parser supports all types that TypeCheck does and even more. It also correctly infers all remote types in @spec.

@orsinium
Copy link
Contributor Author

In theory, we also could use Code.Typespec.spec_to_quoted for convert, but custom implementation gives more control.

Copy link
Owner

@Qqwy Qqwy left a comment

Choose a reason for hiding this comment

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

Stability

I think it would be a good idea to move ensure_types!/apply/fetch_spec and fetch_type to a separate module, say TypeCheck.Experimental to make it even more clear that their API and internals might be subject to change.
(Side note: I still hope to release v1.0 of this library around ElixirConf EU, which is why we should be very clear which parts are and are not yet stable.)

Obviously the 'Experimental' doc sections might then be moved to the moduledoc.

Raising/non-raising

I'm not 100% certain (would love to know your opinion) but currently leaning towards also providing non-raising (error-tuple returning) versions of at least apply! to allow error-recovery without exceptions.


Please don't feel like all my comments have to be fixed right now. A lot of them need to be addressed at some point, but they might wait until after this PR is merged.

Thank you so much for all your hard work on this!
It is absolutely amazing. ❤️‍🔥

lib/type_check.ex Outdated Show resolved Hide resolved
lib/type_check.ex Outdated Show resolved Hide resolved
lib/type_check.ex Outdated Show resolved Hide resolved
lib/type_check.ex Outdated Show resolved Hide resolved
lib/type_check.ex Outdated Show resolved Hide resolved
lib/type_check/internals/parser.ex Outdated Show resolved Hide resolved
lib/type_check/internals/parser.ex Outdated Show resolved Hide resolved
test/type_check/builtin_test.exs Show resolved Hide resolved
test/type_check/internals/parser_test.exs Show resolved Hide resolved
test/type_check/internals/parser_test.exs Outdated Show resolved Hide resolved
@orsinium orsinium changed the title Typespec generics Supprot for built-in @type and @spec May 28, 2022
@Qqwy Qqwy changed the title Supprot for built-in @type and @spec Support for built-in @type and @spec May 28, 2022
@Qqwy
Copy link
Owner

Qqwy commented May 29, 2022

Good idea. I propose to call the module something like "TypeCheck.Classic" and say in its docs that the module is experimental.

What about the name TypeCheck.External? 'Classic' has the connotation of referring to an old version of the code, which I do not think is appropriate here.

'External' indicates that you can use it to work with types of modules which you do not directly have access to yourself.

@orsinium orsinium changed the title WIP Support for built-in @type and @spec Support for built-in @type and @spec May 30, 2022
@orsinium orsinium requested a review from Qqwy May 30, 2022 12:03
@orsinium
Copy link
Contributor Author

orsinium commented May 30, 2022

Changes since the last review:

  1. Support for recursive types, with recursion depth limit
  2. Support for user types
  3. Moved some funcs into TypeCheck.External
  4. Added TypeCheck.apply to return a monad
  5. Support for reference, port, maybe_improper_list
  6. More tests
  7. Simplification of type | any to just any (with any number of types inside).
  8. Simplification of nested union for any number of arguments.
  9. Added ensure_loaded before reading @type and @spec.

@orsinium
Copy link
Contributor Author

A few more changes:

  1. Support for nonempty_maybe_improper_list because nonempty_maybe_improper_list explodes #120 is merged.
  2. Moved apply and apply! into External since you need them only with "external", non-type-check-annotated, functions.

Copy link
Owner

@Qqwy Qqwy left a comment

Choose a reason for hiding this comment

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

The PR is shaping up nicely! 😊
There are only some tiny nitpicks left, most of which are optional.

lib/type_check/external.ex Show resolved Hide resolved
lib/type_check/internals/parser.ex Outdated Show resolved Hide resolved
test/type_check/type_error/formatter_test.exs Show resolved Hide resolved
@orsinium orsinium requested a review from Qqwy May 31, 2022 11:21
Copy link
Owner

@Qqwy Qqwy left a comment

Choose a reason for hiding this comment

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

Wonderful work! ❤️‍🔥

@Qqwy Qqwy merged commit 2a18d16 into Qqwy:master May 31, 2022
@orsinium orsinium deleted the typespec-generics branch May 31, 2022 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants