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

Repeated shadow function definition across documents doesn't trigger error message #19

Open
jwoLondon opened this issue Jan 10, 2019 · 3 comments
Labels
bug Something isn't working priority/p2

Comments

@jwoLondon
Copy link
Member

If you create a document containing the following two functions

```elm {l}
myFunction : String
myFunction =
    let
        repeatedName =
            "Hello"
    in
    repeatedName
```

```elm {r}
repeatedName : String
repeatedName =
    "world"
```

the linter correctly reports the error The name 'repeatedName' is first defined here: repeatedName = But then it is defined AGAIN over here: repeatedName = , as Elm 0.19 does not allow shadowing of functions.

However, if the code block defining repeatedName sits instead in a separate document that follows the one that defines myFunction, the error persists (as we would expect), but no error message is reported (which is a litvis bug).

This is likely to be problematic for beginners or anyone following a document they have not authored themselves, as it is quite a hard error to spot without a linter message.

The problem is that a downstream modification can render an upstream local function in a separate document invalid. This can appear counterintuitive as local functions occupy a global namespace so making it doubly hard to spot without linter help.

@jwoLondon jwoLondon added bug Something isn't working priority/p1 labels Jan 10, 2019
@kachkaev
Copy link
Member

Hmmm I did not know about shadowing in Elm. I'm linting against this rule in JS/TS, but because the code in those languages is split into smaller modules, shadowing is easier to avoid.

When you're saying that the error does not appear in branching narratives, do you mean the document that follows or the one that's being followed? When we are evaluating a litvis document we don't downstream documents, so the latter case would be normal. If it's the first case, then yes, it's a bug (probably related to the fact that Elm line mapping goes out of range while we're in the second document and the error originates from the first one).

@jwoLondon
Copy link
Member Author

To clarify and test. We have two documents, doc1.md looks like this:

```elm {l}
myFunction : String
myFunction =
    let
        repeatedName =
            "Hello"
    in
    repeatedName
```

and doc2.md looks like this:

---
follows: doc1
---

```elm {l}
repeatedName : String
repeatedName =
    "world"
```

If you open both documents in Atom, viewing doc1 appears normal with no error message reported by the linter (consistent with what you said should happen). If you view doc2 an error is indicated in the linter ((!) 1 in red at the bottom), but with no actual error message and no indication of where the error lies. This I think is the problem. Interestingly, if you switch back to doc1, the offending repeatedName line is highlighted for a fraction of a second, then disappears.

If the content of doc1 and doc2 are in in a single document, the linter identifies the problem, highlighting the local instance of repeatedName. This is probably the cause of the problem: The compiler always signals the local shadowing instance as the cause of the problem, but in the arrangement I have described above, that sits in doc1 which previously compiled.

When code includes an attempt at shadowing, it is debatable as to whether conceptually the error is in (a) the first instance of the repeated name; (b) the second instance; (c) the local instance; (d) the global instance or (e) both instances. I think ideally we would report (e) indicating both instances of the repeated name (as Elm does normally). But for consistency, sticking to the principle that once an upstream document (doc1) compiles, nothing downstream should need to re-evaluate it, I would recommend reporting the error in the downstream document (doc2) only. Just as it does when a globally scoped function is repeated (i.e. not shadowed, but a top level function in doc1).

@jwoLondon
Copy link
Member Author

This appears to be an Atom-specific problem. If the two documents are opened in VSCode, error messages are indicated as expected, and clicking on the error takes you to the relevant document (either the first or second depending on which part of the message is selected).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority/p2
Projects
None yet
Development

No branches or pull requests

2 participants