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

Add inference when using dependency injection via module attribute #133

Merged
merged 2 commits into from May 22, 2021
Merged

Conversation

gugahoa
Copy link
Contributor

@gugahoa gugahoa commented May 16, 2021

Implements elixir-lsp/elixir-ls#541

This PR adds basic inference when doing dependency injection using module attributes that calls into Application.compile_env/3, Application.compile_env!/2, Application.get_env/3``and Application.fetch_env!/2`. It works by calling the function and returning the resulting module as the expression type

Add a test case of parsing a module attribute which calls to
`Application.get_env/3` to ensure future implementations that depends on it
does not break
@lukaszsamson
Copy link
Collaborator

Nice @gugahoa, it looks promising. It shouldn't be to difficult to add support for other types as well.

but compile_env is a macro

Oh, did't know that. We can just call fetch/get instead as this is only semantic indicator that it shouldn't be called in runtime.

There's one thing that has been on my mind. I felt that the language server got slower, but I'm not sure if it's a feeling or if it's real. I'll set up a benchmark to see if there's a noticeable difference. If there's, what do you think of moving the expansion to the metadata_builder.ex instead of binding.ex?

I'll do some testing but IMO this feature should not impact other usecases. Regarding the moving, the idea was to keep MetadataBuilder rather simple. Anyway, I wouldn't try it in this PR.

@gugahoa
Copy link
Contributor Author

gugahoa commented May 16, 2021

We can just call fetch/get instead as this is only semantic indicator that it shouldn't be called in runtime.

Didn't think of that! It worked nicely 8b74fea

@gugahoa
Copy link
Contributor Author

gugahoa commented May 17, 2021

I believe it's ready for review. Do you want me to squash the fixups and mark as ready for review or do you want me to wait a bit?

Edit: There's another way of doing dependency injection that would be nice to add inference to:
@some_module Application.get_env(:app, __MODULE__, []) |> Keyword.get(:module, DefaultModule)

But to add support to that we would need to add support to parsing lists, and then add support to calling Keyword.get so I think it should be done in another PR, to keep this one small. What do you think?

@lukaszsamson
Copy link
Collaborator

I think it's ready. @gugahoa do you plan to do any more changes?

There's another way of doing dependency injection that would be nice to add inference to

We could handle it but I'm not convinced we should support every expression possible. So far Binding module cannot handle lists (maps, structs, tuples, atoms and integers).

@lukaszsamson
Copy link
Collaborator

@gugahoa Please look into test failures

@gugahoa gugahoa marked this pull request as ready for review May 18, 2021 22:59
@gugahoa
Copy link
Contributor Author

gugahoa commented May 18, 2021

@lukaszsamson fixed the tests, now the PR is ready for review

@lukaszsamson lukaszsamson merged commit 1fb9b6b into elixir-lsp:master May 22, 2021
@lukaszsamson
Copy link
Collaborator

Thanks @gugahoa

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