elm / compiler

Compiler for Elm, a functional language for reliable webapps.
https://elm-lang.org/
BSD 3-Clause "New" or "Revised" License
7.54k stars 664 forks source link

Exposed imported variables can be shadowed #1997

Open lydell opened 5 years ago

lydell commented 5 years ago

Quick Summary: Elm does not allow shadowing: https://github.com/elm/compiler/blob/master/hints/shadowing.md But is is possible to shadow exposed imported variables.

SSCCE

module Main exposing (..)

import List exposing (product)
-- or:
-- import List exposing (..)

product =
    { name = "Toothbrush"
    , price = 2
    }

The above does not produce any compile (shadowing) errors!

Additional Details

I think the ideas outlined in https://github.com/elm/compiler/blob/master/hints/shadowing.md applies to shadowing imported variables too. Let’s make a similar example to the name example from that document.

Let’s say were working on AwesomeDental’s website:

import Company exposing (name)
import Html

view =
    Html.h1 [] [ Html.text ("Welcome to " ++ name ++ "’s website!") ]

Some time later, hundreds of lines further down, someone is making a couple of view functions for the company’s new product, the SuperToothbrush. The programmer has mentioned the string "SuperToothbrush" over and over, but now their boss says it’s very important to have a sign after the name. Since finding and updating all the occurances was a bit annoying, the programmer extracts a variable so that a future rename would be easier:

name =
    "SuperToothbrush™"

Oops! Now the website heading will say “Welcome to SuperToothbrush™’s website” rather than “Welcome to AwesomeDental’s website!”

lydell commented 5 years ago

Consideration: If this change is made, this code wouldn’t compile anymore:

someResult |> Result.mapError (\e -> "Error: " ++ e)

Because the e would conflict with Basics.e. But I guess one-letter variable names aren’t that good anyway? I’d rename to err or error or errorMessage in the above example.

Oh, and I should mention the example from the Slack conversation as well. This is forbidden (conflict with Basics.round):

import Round exposing (round)

But this is OK:

import Round

round = Round.round

Which is hard to explain to a beginner.

lydell commented 5 years ago

Another consideration: If this change is made, then the Round package wouldn’t be able to provide the Round.round function anymore, because it wouldn’t be able to create it in the first place (conflicts with Basics.round). Hmm 🤔

rlefevre commented 5 years ago

This is on purpose, see https://github.com/elm/compiler/issues/1806.

lydell commented 5 years ago

Thanks!

I might still be worth disallowing shadowing when an explicit exposing list is used? I mean, if I have bothered typing exposing (length) then why would I ever want to do length x = ...? In other words, only allow shadowing of variables coming from exposing (..).

pravdomil commented 4 years ago

This is on purpose, see #1806.

https://github.com/elm/compiler/blob/master/hints/shadowing.md should mention that