Handle blank mix target value properly

[mnishiguchi]

This addresses an empty string can get passed into Mix.target/1, which sets the Mix target value to :"". Mix target is supposed to default to :host but Mix.target/1 force-updates it with any wrong value. We should make sure the target value is valid before converting it to atom.

The accidental :"" is bad, because for example, Nerves projects assume Mix.target/0 defaults to :host.

Notes

• discussed with @fhunleth at Mix.target() is :"" for ElixirLS nerves-project/nerves#694
• I attempted to improve Mix.target/1 in Elixir language first, but Jose says it is not Elixir's responsibility. Handle blank Mix target values properly elixir-lang/elixir#11603

[fhunleth]

I think that I'm misunderstanding this PR. I would not expect elixir-ls to set the Mix target as :host if it is unset. It would be better if the code at https://github.com/elixir-lang/elixir/blob/main/lib/mix/lib/mix/state.ex#L22-L28 could set the default. Does using elixir-ls preclude mix from applying the MIX_TARGET default value?

[mnishiguchi]

@fhunleth I tried to address it in Elixir, but Jose does not want it there. elixir-lang/elixir#11603

In the code in question, Elixir LS wants to force-update the Mix.target value using Mix.target/1, in which Elixir does not want to validate the provided value. When the value is "", Elixir would use :"" as Mix.target.

Maybe there is a way to convince Jose?

[fhunleth]

I agree with Jose. Why can't the existing Elixir code that sets the default be used?

[lukaszsamson]

Can't we opt out of setting target when it's null or ""?

[jjcarstens]

This seems partly due to a change in VScode or some other underlying way of getting settings as mixTarget is no longer allowing null values, so it is always set to "" or a user value.

I think you can adjust this line to include "" in the check and no other changes would be needed:
https://github.com/elixir-lsp/elixir-ls/blob/master/apps/language_server/lib/language_server/server.ex#L1158

[jjcarstens]

I also think changing this minLength in VScode setting would also help prevent an empty "" from being used in the target and incorrectly setting it
https://github.com/elixir-lsp/vscode-elixir-ls/blob/master/package.json#L120

[mnishiguchi]

@lukaszsamson @jjcarstens I simplified the changes base on your feedback. What do you guys think?

• handle "" in maybe_set_mix_target/1
• remove unnecessary operation (target || "host") in set_mix_target/2

[fhunleth]

I like this code. This looks like it's a problem with the current code too, but if you have a target set and then you unset it, don't you need to call Mix.target(nil)? It looks like if the target is set to nil or "" that the call to set the target is skipped.

[fhunleth]

I was told offline that the issue I was worried about is not a problem since Elixir LS restarts Mix when the setting changes. Thank you all at Elixir LS for the super useful tool and thanks @mnishiguchi for fixing the :"" Mix target issue.

[jjcarstens]

👍 LGTM!