Deprecated unquoted strings

[Malian]

Related to #340

My first attempt was to add handle this in the parser but I believe it is not the role of the parser and it. I was able to move this warning to the TypeHandler or directly to the Compiler. From my point of view, it makes more sense to handle it in the compiler, so I decided to catch it in the attr_value function when the attr_value is not a string. If it is not a string, even if the type is :string, it means that the value as been converted before to something else, boolean or integer.

Ok, this will teach me not to run all the tests before pushing...

[Malian]

Mhhh these tests should not pass. We should force warning_as_error=true in 0.5

[Malian]

@msaraiva & @lnr0626 due to the parser we will have to raise a warning from the parser, or modify the way it handles <div selected>.

Because of this piece of code in the parser:

surface/lib/surface/compiler/parser.ex

Lines 229 to 231 in 2a17bda

	defp translate_attr({name, nil, meta}) do
	{name, true, to_meta(meta)}
	end

There is no way to differentiate <div selected> from <div selected=true> in the compiler.

We could emit a warning from the parser, but we do not have access to a caller for IOHelper.warn. We could also modify the translate_attr signature to differentiate both cases.

For example, we could replace the above function with:

  defp translate_attr({name, nil, meta}) do
    {name, {:attribute_expr, "true", to_meta(meta)}, to_meta(meta)}
  end

and keep the current implementation. It means that <div selected> === <div selected={true}> !== <div selected=true>. Then in the compiler we emit a warning when a :string type with a boolean value or an integer value are provided.

WDYT? Do you have any alternatives?

[msaraiva]

@Malian what about passing an extra field in the attribute's metadata to differentiate them? Since the attribute's metadata is available in attr_value, you could check that info.

[Malian]

what about passing an extra field in the attribute's metadata to differentiate them? Since the attribute's metadata is available in attr_value, you could check that info.

@msaraiva It is a solution, but it implies that we have to modify the AST.Meta struct to accept extra metadata about the node. We call everywhere in the code Helpers.to_meta that returns an AST.Meta. Adding an attribute that is only used for deprecated code and that we will remove in the future seems weird.

surface/lib/surface/compiler/helpers.ex

Lines 101 to 113 in f3a6d22

	def to_meta(tree_meta, %CompileMeta{caller: caller, checks: checks}) do
	%AST.Meta{
	line: tree_meta.line,
	column: tree_meta.column,
	file: tree_meta.file,
	caller: caller,
	checks: checks
	}
	end
	def to_meta(tree_meta, %AST.Meta{} = parent_meta) do
	%{parent_meta | line: tree_meta.line, column: tree_meta.column}
	end

We could add a field that handles all extra meta, but It is maybe an overkill addition at this stage, isn't it?

An alternative could be to catch this extra metadata one function above in the process_attributes but I have the feeling that it makes the code more spaghetti. As I said, we will remove this code in the future, so maybe we should not care too much about it?

surface/lib/surface/compiler.ex

Lines 607 to 611 in f3a6d22

	defp process_attributes(mod, [{name, value, attr_meta} | attrs], meta, acc) do
	name = String.to_atom(name)
	attr_meta = Helpers.to_meta(attr_meta, meta)
	{type, type_opts} = Surface.TypeHandler.attribute_type_and_opts(mod, name, attr_meta)

[msaraiva]

we will remove this code in the future, so maybe we should not care too much about it?

Agreed. We shouldn't care. Those deprecations are temporary so if they work as expected, it's fine. 👍