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

Import suggestion for missing newtype constructor, all types constructor and indirect overloadedrecorddot fields #4516

Merged
merged 7 commits into from
Apr 4, 2025

Conversation

guibou
Copy link
Collaborator

@guibou guibou commented Mar 11, 2025

This MR contains 3 commits which improves the import suggestion code actions:

  • suggests to import a module when the newtype constructor is not in scope. This usually happen when coerceing newtype.
  • suggests to import a module when all the data constructors for a type are not in scope. This usually happen when creating an instance of a type from another module. For example, deriving instance Generic Bar when Bar (the type) may be in scope, but not the construtors (e.g. requiring Bar(..)).
  • suggests to import a module when a field of a type is not in scope and used in an OverloadedRecordDot context, such as foo.bar where foo :: Foo and Foo(bar) is not in scope. Note that this is already partially handled by HLS in the case where the module containing Foo is already imported, but not the fields. In this context, GHC is kind enough to give us an hint (something like "maybe add field bar to import line...") and this is already handled by HLS. However, if the module containing Foo is not already imported, it does not work.

All of these case usually happen when types are "indirectly" in scope. For example, consider the followings:

module T where

data Foo = Foo { foo :: Int }
module Value where
import T

foo = Foo 10
module Usage where

import Value

result = foo.foo

In the module Usage, the type Foo is not explicitly imported, it exists just indirectly because foo is imported from Value.

Discussion

I tried to take care of multiples patterns in the regex used to match the error messages, such as qualified or not qualified types or class, as well as polymorphic or not. If you see a case not taken into account, please tell me.

Tests

I've added simple tests for the coerce and instance Generic "cases" and a more involved test for the overloaded record dot cases.

I've developed this using GHC 9.10. I'll test on other version of GHC asap, perhaps some adaptations of the regex need to be introduced.

@guibou guibou requested a review from santiweight as a code owner March 11, 2025 15:23
@fendor fendor added the status: needs review This PR is ready for review label Mar 11, 2025
@guibou guibou force-pushed the import_suggestion_for_coerce branch from bddf4b5 to f1eb36a Compare March 12, 2025 08:13
@guibou
Copy link
Collaborator Author

guibou commented Mar 12, 2025

Rebased and fixed test for GHC 9.4.

Copy link
Collaborator

@soulomoon soulomoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change ~
I think we just need more test on the regex.
Otherwise it is good to go.

@guibou guibou force-pushed the import_suggestion_for_coerce branch 2 times, most recently from 01ce436 to 2b27964 Compare April 1, 2025 06:56
@guibou guibou requested a review from soulomoon April 1, 2025 10:06
@guibou
Copy link
Collaborator Author

guibou commented Apr 1, 2025

I've added a new commit which fixs (and add tests) for not in scope record field.

This was working fine with ghc <9.10 when the message was Not in scope: 'foo' but since it is now Not in scope: record field 'foo' the result was that record was matched instead of foo.

see:

image

The regex associated with Not in scope are fragile and maybe there are other not in scope which are infortunately matched incorrectly.

@guibou guibou force-pushed the import_suggestion_for_coerce branch from ee52849 to 282e820 Compare April 1, 2025 12:06
@guibou
Copy link
Collaborator Author

guibou commented Apr 1, 2025

I'm not happy. Looks like I hit the difference in GHC 9.10 (and 9.8) about in which context fields name are found.

Even if the NotInScopeTHing contains the right "foo" naming, it is not able to locate this value inside the map of indexed symbols.

I'll investigate a bit more.

@guibou guibou force-pushed the import_suggestion_for_coerce branch from cb9d4e3 to dcb6d3a Compare April 3, 2025 11:19
@guibou
Copy link
Collaborator Author

guibou commented Apr 3, 2025

I finally found the solution (I hope), which is to use lookupOccEnv_AllNameSpaces in order to look for symbol in all namespaces, including field selector namespaces.

I've done this refactor, added a bit more tests, tell me what you think.

Note that, maybe in a future refactor, we could try to be a bit more selective in the way we are looking for import. The NotInScope thing may contain more information that what we currently use. It contains if the symbol looked for is a type, a data, a variable, and can also be extended in order to contain the field selector information, and the lookup function can be extended to take that into account, leading to less conflicts (e.g. a missing record selector toto won't suggest the import of the function toto).

Note also that some of the tests in this MR, related to field selector, are just testing that we can suggest to import the Type name (e.g. import Module (Foo(..)) and could be extended to add support for selecting the field (when both type and field name are present, such as is HasField instance error) and the test could also be extended to test the different configuration. But maybe that's a bit outside of the scope of this MR.

@guibou
Copy link
Collaborator Author

guibou commented Apr 3, 2025

image

With ghc 9.10 ;)

@fendor fendor requested review from jhrcek and wz1000 April 3, 2025 18:02
@soulomoon
Copy link
Collaborator

Note that, maybe in a future refactor, we could try to be a bit more selective in the way we are looking for import.

Maybe we can open issue to track the things we would want to do for imports. And hopefully tackle them one by one.

Copy link
Collaborator

@soulomoon soulomoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, @guibou!
This looks good to me now — a solid improvement to the current import system.
Personally, I use import suggestions quite frequently, so this is a very welcome enhancement.

guibou added 7 commits April 4, 2025 13:21
In a context where we want to `coerce` to a type for which constructor
is not in scope, GHC fails with (e.g., for `Sum Int`):

```
• Couldn't match representation of type ‘Int’ with that of ‘Sum Int’
    arising from a use of ‘coerce’
    The data constructor ‘base-4.18.2.1:Data.Semigroup.Internal.Sum’
      of newtype ‘Sum’ is not in scope
```

This code action detects the missing `newtype` and suggests to add the
required import.

This is convenient because otherwise the user need to interpret the
error message and most of the time manually find which module and type to import.

Note that a better implementation could try to decet that the type is
already imported (if that's the case) and just suggest to add the
constructor (e.g. `(..)`) in the import list, but this is too much
complexity to implement. It could lead to duplicated import lines which
will be "cleaned" by formatter or other extensions.
For example, `deriving instance Generic (Sum Int)`, but it should work
for other deriving which indirectly requires a complete access to the
type constructor.

```
Can't make a derived instance of ‘Generic (Sum Int)’:
    The data constructors of ‘Sum’ are not all in scope
      so you cannot derive an instance for it
```
For example, the following code `foo.titi` when the type of `foo` (e.g.
`Bar` here is not in scope and not from an already imported module (e.g.
the type exists indirectly because here `foo :: Bar` comes from another module).

If the module which contains `Bar` is already imported, GHC already
gives an hint to add `titi` to the `import Bar` line and this is already
correctly handled by HLS.

```
 No instance for ‘HasField "titi" Bar.Bar String’
    arising from selecting the field ‘titi’
```
This correct previous commit by handling ghc 9.4 parethensis instead of
"tick".
In ghc 9.6 we had:

```
Not in scope ‘modelStatusTag’
```

In 9.10 we have:

```
Not in scope: record field ‘modelStatusTag’
```

Introducing the new regex in order to match it AND a test to ensure no
future regression.

The regression is due to the fact that there is a matcher which catch
`Nat in scope: .*`, hence it was matching "record" instead of
"modelStatusTag".
In GHC >= 9.8, the namespace for record selector changed and is now part
of a new namespace. This allows for duplicated record field names in the
same module.

This hence generated a few issues in HLS when looking for a symbol using
`lookupOccEnv`: the current implementation was only doing lookup in
type, data and var namespaces.

This commit uses `lookupOccEnv_AllNameSpaces`, so it may be more
efficient (one lookup instead of two), but it also incluse the symbols
from record field selectors and this will actually fix most import
suggestion logic when a record field selector is not found.

Note that the function is not available in `ghc` <= 9.6, hence the `CPP`
and fallsback implementation, which uses the previous implementation.

See https://gitlab.haskell.org/ghc/ghc/-/wikis/migration/9.8#new-namespace-for-record-fields
@guibou guibou force-pushed the import_suggestion_for_coerce branch from dcb6d3a to dc6a9e3 Compare April 4, 2025 09:21
@guibou guibou enabled auto-merge (rebase) April 4, 2025 09:21
@guibou guibou merged commit 4edf9cd into master Apr 4, 2025
41 checks passed
@guibou guibou deleted the import_suggestion_for_coerce branch April 5, 2025 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants