exercism / elixir-analyzer

GNU Affero General Public License v3.0
30 stars 31 forks source link

Relax german-sysadmin integer literal check #389

Closed angelikatyborska closed 11 months ago

angelikatyborska commented 1 year ago

Resolves https://github.com/exercism/elixir-analyzer/issues/384

The check is relaxed - now just one usage of , , , , ?_, ?a, or ?z in the solution is enough, as long as the corresponding integer literals aren't found.

A trade-off: this will produce false-positives in scenarios in which people randomly include one of those numbers in the code, including in comments, e.g. ?ß # 223

dabaer commented 1 year ago

RE the false positives, you could reduce the surface area by removing comments from the checked code:

check(%Source{code_string: code_string}) do
  question_mark_code_points = ["?ß", "?ä", "?ö", "?ü", "?_", "?a", "?z"]
  integer_literal_code_points = ["223", "228", "246", "252", "95", "97", "122"]

  code_string = code_string |> Code.string_to_quoted!() |> Macro.to_string()

  Enum.any?(question_mark_code_points, &String.contains?(code_string, &1)) &&
    not Enum.any?(integer_literal_code_points, &String.contains?(code_string, &1))
end

My example was slightly na?ïve I suppose, an optimization would be to use the code_ast instead of code_string and skip the step of converting the already converted string back into an AST.

angelikatyborska commented 11 months ago

@dabaer The reason we're doing this check on the string in the first place is the same reason your suggestion wouldn't work: the ? syntax is just syntactic sugar that gets completely stripped when producing an AST:

iex(6)> "if x == ?A, do: ?B, else: 0"
"if x == ?A, do: ?B, else: 0"

iex(7)> v |> Code.string_to_quoted!()
{:if, [line: 1],
 [{:==, [line: 1], [{:x, [line: 1], nil}, 65]}, [do: 66, else: 0]]}

iex(8)> v |> Macro.to_string()
"if(x == 65) do\n  66\nelse\n  0\nend"
angelikatyborska commented 11 months ago

@jie like this 7839ac0 (#389) ?

jiegillet commented 11 months ago

@angelikatyborska yes, looks good! (you called the wrong jie in the process though haha)

angelikatyborska commented 11 months ago

Hahaha, oh no 🙈 that explains why you didn't react