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

Add source position information for missing imports #812

Merged
merged 4 commits into from
Feb 8, 2019

Conversation

Gabriella439
Copy link
Collaborator

Related to #561

This adds source position information to missing imports

Before:

$ dhall <<< './foo'

↳ ./foo

Error: Missing file …/foo

After:

$ dhall <<< './foo'

↳ ./foo

Error: Missing file …/foo

(stdin):1:1

Related to #561

This adds source position information to missing imports

Before:

```
$ dhall <<< './foo'

↳ ./foo

Error: Missing file …/foo
```

After:

```
$ dhall <<< './foo'

↳ ./foo

Error: Missing file …/foo

(stdin):1:1
```
@Profpatsch
Copy link
Member

Awesome, I think I can work with this. Now my next request would be to show a NonEmpty of all missing imports. I’d like to see you do that with exceptions. :P

@Gabriella439
Copy link
Collaborator Author

@Profpatsch: Yeah, that will follow in another pull request (possibly replacing exceptions with something like ExceptT along the way)

@Profpatsch
Copy link
Member

possibly replacing exceptions with something like ExceptT along the way

I approve of this. It’s so darn hard to follow the exception codepaths.

@Gabriella439 Gabriella439 merged commit 98497e4 into master Feb 8, 2019
@Gabriella439 Gabriella439 deleted the gabriel/source_missing_import branch February 8, 2019 15:32
@Profpatsch
Copy link
Member

Ah, the position of the imports in the source files are not yet available though, I get 1:1:(source) when running this patch:

foo.dhall

{ aetiglnetglineeglseglvcnvfglcnlvfgneqlenlvqgenlvqgenleqgvnle =
	{ xyc = ./abc }
}
$ dhall-flycheck foo.dhall
[{"filename":"foo.dhall","line":1,"column":1,"message":"\n\n↳ ./abc\n\nError: Missing file /home/philip/kot/dhall/flycheck/abc\n\n"}]

I would expect the error position to point to the file name on line 2 of foo.dhall. That way I can show the problematic file.

Oh, and if I have

bar.dhall

./foo.dhall
$ dhall-flycheck bar.dhall
[{"filename":"bar.dhall","line":1,"column":1,"message":"\n\n↳ ./foo.dhall\n  ↳ ./abc\n\nError: Missing file /home/philip/kot/dhall/flycheck/abc\n\n"}]

I need some way to point to the import of ./foo.dhall in bar.dhall to highlight the transitive import problem (so that the user can follow the import chain though until the import of the missing file itself is highlighted).

@Profpatsch
Copy link
Member

Something like { foo = ./foo.dhall, abc = ./abc } should highlight both ./foo.dhall (because of the transitive import error of abc) as well as ./abc (because of the direct import error) of course.

Gabriella439 added a commit that referenced this pull request Feb 12, 2019
.. as caught by @Profpatsch in:

#812 (comment)

Before this change the location was always being reported as `(stdin):1:1`
because the `SourcedException` kept getting modified with a broader
source location in the `Note` branch of `loadWith`.

This was originally done so that alternative imports would show the entire
source span, but it mistakenly just kept bubbling up regardless of whether or
not there were alternative imports.

Instead, this includes an approximate source span for alternative imports.
The source span bounds are correct but the contents just show which imports
were alternated, even if they might have been buried in a larger expression.

For example, if the original expression had been:

```haskell
Some ./x ? None ./y
```

... then the source span for the error message would display just:

```haskell
./x ? ./y
```

... which is probably as good as it will get for now.
Gabriella439 added a commit that referenced this pull request Feb 12, 2019
.. as caught by @Profpatsch in:

#812 (comment)

Before this change the location was always being reported as `(stdin):1:1`
because the `SourcedException` kept getting modified with a broader
source location in the `Note` branch of `loadWith`.

This was originally done so that alternative imports would show the entire
source span, but it mistakenly just kept bubbling up regardless of whether or
not there were alternative imports.

Instead, this includes an approximate source span for alternative imports.
The source span bounds are correct but the contents just show which imports
were alternated, even if they might have been buried in a larger expression.

For example, if the original expression had been:

```haskell
Some ./x ? None ./y
```

... then the source span for the error message would display just:

```haskell
./x ? ./y
```

... which is probably as good as it will get for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants