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

Warning when consolidating Inspect protocol with redacted fields #3464

Closed
crbelaus opened this issue Oct 29, 2020 · 15 comments
Closed

Warning when consolidating Inspect protocol with redacted fields #3464

crbelaus opened this issue Oct 29, 2020 · 15 comments

Comments

@crbelaus
Copy link
Contributor

crbelaus commented Oct 29, 2020

Environment

  • Elixir version (elixir -v):
Erlang/OTP 23 [erts-11.1.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe]
Elixir 1.11.1 (compiled with Erlang/OTP 21)
  • Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.):
PostgreSQL 12.4
  • Ecto version (mix deps):
* ecto 3.5.4 (Hex package) (mix)
  locked at 3.5.4 (ecto) 7f13f9c9
  ok
* ecto_sql 3.5.3 (Hex package) (mix)
  locked at 3.5.3 (ecto_sql) d2f53592
  ok
  • Database adapter and version (mix deps):
* postgrex 0.15.7 (Hex package) (mix)
  locked at 0.15.7 (postgrex) 88310c01
  ok
  • Operating system:
Ubuntu 20.04

Current behavior

Creating a schema with a redacted field causes a warning when the project is compiled by ElixirLS. I've first encountered this problem in a personal project and was able to track it down to the redacted: true property on the field.

I've created an example repository to reproduce this behaviour. After loading the project, any change done in the EctoWarningExample.User module that triggers a recompilation will cause this warning to appear in ElixirLS. This can be tested simply by adding a new blank line into the module.

I've taken a look into the Ecto code and found this. It looks like Ecto is deriving the Inspect protocol for this schema to avoid showing the redacted field. What I don't really know is where is the other implementation for the same protocol which is triggering the warning.

Screenshot of the warning in VSCode with ElixirLS

image

The warning message itself

the Inspect protocol has already been consolidated, an implementation for EctoWarningExample.User has no effect. If you want to implement protocols after compilation or during tests, check the "Consolidation" section in the Protocol module documentation

Expected behavior

The code should compile cleanly without warnings.

@crbelaus
Copy link
Contributor Author

I've initally opened a thread with this behaviour in ElixirForum since I didn't know if it was a bug or I had something wrong in my code. As can bee seen in the responses other people seems to experience the same problem, so I've decided to open it as an issue here.

@josevalim
Copy link
Member

Do you see the warning only on ElixirLS? If so, which Elixir LS version are you using? Notice there is a fork which is more recently updated. Please let me know if the issue persists there too.

@crbelaus
Copy link
Contributor Author

I am using the following ElixirLS, which I believe is the latest one after the fork was merged with the "official" extension:

Name: ElixirLS: Elixir support and debugger
Id: jakebecker.elixir-ls
Description: Elixir support with debugger, autocomplete, and more. Powered by ElixirLS.
Version: 0.6.1
Publisher: ElixirLS
VS Marketplace Link: https://marketplace.visualstudio.com/items?itemName=JakeBecker.elixir-ls

@josevalim
Copy link
Member

Ah, the fork has been deprecated. I am behind the times. :) @msaraiva, can you reproduce this?

@lukaszsamson
Copy link
Contributor

@josevalim I can reproduce it one of my projects on elixirLS but not in mix. This particular project does not depend on ecto so I believe it's a generic issue. The code that generates this warning defines a protocol and many structs implementing it with @derive. I hadn't got the time to debug it yet though.

@crbelaus
Copy link
Contributor Author

crbelaus commented Nov 1, 2020

I've tried some previous versions of ElixirLS (0.50 and 0.40) and I can reliably reproduce this behaviour in both of them.

I am unsure about how to debug this, but if we all agree that this seems to be a ElixirLS bug maybe I should move this issues to their repository.

@sb8244
Copy link
Contributor

sb8244 commented Nov 4, 2020

I am using VSCode with ElixirLS, but I always run an embedded terminal with iex -S mix phx.server during development. When it recompiles, I get the mentioned error for all schemas that have redact: true in them.

I get the error once per redacted field in the iex session. For example, I have a schema with 2 redacted fields, and I get 2 back-to-back warnings. Like so:

Compiling 33 files (.ex)
warning: the Inspect protocol has already been consolidated, an implementation for Clove.Connections.Schema.SalesforceConnection has no effect. If you want to implement protocols after compilation or during tests, check the "Consolidation" section in the Protocol module documentation
  lib/clove/connections/schema/salesforce_connection.ex:9: Clove.Connections.Schema.SalesforceConnection (module)

warning: the Inspect protocol has already been consolidated, an implementation for Clove.Connections.Schema.SalesforceConnection has no effect. If you want to implement protocols after compilation or during tests, check the "Consolidation" section in the Protocol module documentation
  lib/clove/connections/schema/salesforce_connection.ex:9: Clove.Connections.Schema.SalesforceConnection (module)

As far as I can tell, I don't get the error in ElixirLS. I am looking at the "output" screen in VSCode and see no reference to the warning.

I attempted to disable consolidation but the warnings still appear. Am I misunderstanding something about this option?:

  def project do
    [
      ...,
      consolidate_protocols: Mix.env() != :dev
    ]
  end

@josevalim
Copy link
Member

The fact you can reproduce it without ElixirLS is great. Any chance you isolate it to a small project that you can share so I take a look?

And to be double sure... you are not manually implementing inspect for these schemas, right?

@crbelaus
Copy link
Contributor Author

crbelaus commented Nov 4, 2020

@sb8244 @josevalim I can reproduce the behaviour in the example project provided in the issue description.

Steps:

  1. Clone the project and install all dependencies
  2. Start the server with iex -S mix phx.server
  3. Open http://localhost:4000/ in your browser
  4. Make any change in lib/ecto_warning_example/user.ex. Even adding a blank line works.
  5. Refresh your browser so the application recompiles the last change
  6. Take a look at the server log and you will see the warning.

@josevalim
Copy link
Member

This is a Phoenix bug. I have pushed a fix to Phoenix v1.5 and master branches. Please give one of them a try and let me know if you find anything else, thanks!

@altdsoy
Copy link

altdsoy commented Nov 4, 2020

This is a Phoenix bug. I have pushed a fix to Phoenix v1.5 and master branches. Please give one of them a try and let me know if you find anything else, thanks!

In my case I tested with the :git dependency of phoenix, and indeed the warning disappeared right away..

For reference I only did mix deps.clean phoenix --unlock and then changed the :phoenix line like so
:phoenix, git: "https://github.com/phoenixframework/phoenix", override: true},
(overridde: true because of the dependency to phoenix 1.5.2 of phx_gen_auth)

Thanks for the quick fix José!

@axelson
Copy link
Contributor

axelson commented Nov 4, 2020

For future reference this appears to be the commit that fixes this issue: phoenixframework/phoenix@b5580e9

@crbelaus
Copy link
Contributor Author

crbelaus commented Nov 5, 2020

I can confirm that the bug is fixed in Phoenix master 👍 I can not reproduce it anymore with the example repository for this bug.

@DavidVII
Copy link

DavidVII commented Jun 2, 2022

I wasn't sure where to post this issue, but google led me here and I'm hoping it's ok.

I'm noticing this issue after upgrading to Elixir 1.12.3 -> 1.13.4 and Phoenix 1.6.6 -> 1.6.10 when I run the following:

$ mix xref trace lib/ros/accounts/user.ex
> warning: the Inspect protocol has already been consolidated, an implementation for Ros.Accounts.User has no effect. If you want to implement protocols after compilation or during tests, check the "Consolidation" section in the Protocol module documentation
  lib/ros/accounts/user.ex:11: Ros.Accounts.User (module)

The code looks like this:

10  @derive {Inspect, except: [:password]}
11  schema "users" do

I've read the referenced section and it's unclear to me if this is an actual issue or if I can safely ignore it.

Has anyone here experienced this issue recently? I ran mix deps.clean and the warning persists.

@josevalim
Copy link
Member

Can you please open up an issue in Elixir? This is safe but we should either document this in mix trace or change mix trace to avoid this warning. :)

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

No branches or pull requests

7 participants