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.

Sign up for GitHub

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

Fix -Wall in qualified imported names plugin #4070

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Fix -Wall in qualified imported names plugin #4070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions haskell-language-server.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,7 +921,7 @@ common qualifyImportedNames
cpp-options: -Dhls_qualifyImportedNames

library hls-qualify-imported-names-plugin
import: defaults, warnings
import: defaults, pedantic, warnings
exposed-modules: Ide.Plugin.QualifyImportedNames
hs-source-dirs: plugins/hls-qualify-imported-names-plugin/src
build-depends:
Expand All @@ -939,7 +939,7 @@ library hls-qualify-imported-names-plugin
DataKinds

test-suite hls-qualify-imported-names-plugin-tests
import: defaults, test-defaults, warnings
import: defaults, pedantic, test-defaults, warnings
type: exitcode-stdio-1.0
hs-source-dirs: plugins/hls-qualify-imported-names-plugin/test
main-is: Main.hs
Expand Down
26 changes: 11 additions & 15 deletions plugins/hls-qualify-imported-names-plugin/src/Ide/Plugin/QualifyImportedNames.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,8 @@ import Development.IDE.GHC.Compat (ContextInfo (Use),
ImpDeclSpec (ImpDeclSpec, is_as, is_dloc, is_qual),
ImportSpec (ImpSpec),
LImportDecl, ModuleName,
Name, NameEnv, OccName,
ParsedModule, RefMap, Span,
SrcSpan,
Name, NameEnv, ParsedModule,
RefMap, Span, SrcSpan,
TcGblEnv (tcg_rdr_env),
emptyUFM, globalRdrEnvElts,
gre_imp, gre_name, locA,
Expand Down Expand Up @@ -111,7 +110,7 @@ data ImportedBy = ImportedBy {
}

isRangeWithinImportedBy :: Range -> ImportedBy -> Bool
isRangeWithinImportedBy range (ImportedBy _ srcSpan) = fromMaybe False $ spanContainsRange srcSpan range
isRangeWithinImportedBy range ImportedBy{importedBySrcSpan} = fromMaybe False $ spanContainsRange importedBySrcSpan range
Copy link
Collaborator Author

@jhrcek jhrcek Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agree that using named field puns could be controversial, I'm just fixing warnings of the type
Defined but not used: ‘importedBySrcSpan’

Other ways to make this go away:

  • prefix record selectors in data type definition with _
  • use the record selectors (e.g. importedBySrcSpan ib) in function body

Also agree that these are inconsequential warnings, but I'm really close to fixing all the warnings/enabling pedantic flag everywhere and don't want to disable warnings selectively just because these last few small things 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is fine, and I don't think we have any policy regarding NamedFieldPuns.


globalRdrEnvToNameToImportedByMap :: GlobalRdrEnv -> NameEnv [ImportedBy]
globalRdrEnvToNameToImportedByMap =
Expand Down Expand Up @@ -168,9 +167,6 @@ refMapToUsedIdentifiers = DList.toList . Map.foldlWithKey' folder DList.empty
, Use `elem` identInfo = Just $ UsedIdentifier name identifierSpan
| otherwise = Nothing

occNameToText :: OccName -> Text
occNameToText = Text.pack . occNameString

updateColOffset :: Int -> Int -> Int -> Int
updateColOffset row lineOffset colOffset
| row == lineOffset = colOffset
Expand All @@ -182,13 +178,13 @@ usedIdentifiersToTextEdits range nameToImportedByMap sourceText usedIdentifiers
State.evalState (makeStateComputation sortedUsedIdentifiers) (Text.lines sourceText, 0, 0)
where
folder :: [TextEdit] -> UsedIdentifier -> State ([Text], Int, Int) [TextEdit]
folder prevTextEdits (UsedIdentifier identifierName identifierSpan)
| Just importedBys <- lookupNameEnv nameToImportedByMap identifierName
, Just (ImportedBy alias _) <- find (isRangeWithinImportedBy range) importedBys
, let IdentifierSpan row startCol endCol = identifierSpan
, let identifierRange = identifierSpanToRange identifierSpan
, let aliasText = Text.pack $ moduleNameString alias
, let identifierText = Text.pack $ occNameString $ nameOccName identifierName
folder prevTextEdits UsedIdentifier{usedIdentifierName, usedIdentifierSpan}
| Just importedBys <- lookupNameEnv nameToImportedByMap usedIdentifierName
, Just ImportedBy{importedByAlias} <- find (isRangeWithinImportedBy range) importedBys
, let IdentifierSpan row startCol _ = usedIdentifierSpan
, let identifierRange = identifierSpanToRange usedIdentifierSpan
, let aliasText = Text.pack $ moduleNameString importedByAlias
, let identifierText = Text.pack $ occNameString $ nameOccName usedIdentifierName
, let qualifiedIdentifierText = aliasText <> "." <> identifierText = do
(sourceTextLines, lineOffset, updateColOffset row lineOffset -> colOffset) <- State.get
let lines = List.drop (row - lineOffset) sourceTextLines
Expand Down Expand Up @@ -219,7 +215,7 @@ usedIdentifiersToTextEdits range nameToImportedByMap sourceText usedIdentifiers
-- 3. For each used name in refMap check whether the name comes from an import
-- at the origin of the code action.
codeActionProvider :: PluginMethodHandler IdeState Method_TextDocumentCodeAction
codeActionProvider ideState pluginId (CodeActionParams _ _ documentId range context) = do
codeActionProvider ideState _pluginId (CodeActionParams _ _ documentId range _) = do
normalizedFilePath <- getNormalizedFilePathE (documentId ^. L.uri)
TcModuleResult { tmrParsed, tmrTypechecked } <- runActionE "QualifyImportedNames.TypeCheck" ideState $ useE TypeCheck normalizedFilePath
if isJust (findLImportDeclAt range tmrParsed)
Expand Down
Loading