Skip to content

Commit

Permalink
Do not show locations of builtins.
Browse files Browse the repository at this point in the history
  • Loading branch information
athas committed Apr 14, 2022
1 parent a255fb7 commit 919d686
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 7 deletions.
15 changes: 9 additions & 6 deletions src/Futhark/LSP/Tool.hs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import Futhark.LSP.State (State (..))
import Futhark.Util.Loc (Loc (Loc, NoLoc), Pos (Pos), SrcLoc, locOf, srclocOf)
import Futhark.Util.Pretty (prettyText)
import Language.Futhark.Core (locStr)
import Language.Futhark.Prop (isBuiltin)
import Language.Futhark.Prop (isBuiltinLoc)
import Language.Futhark.Query
( AtPos (AtName),
BoundTo (BoundTerm),
Expand All @@ -36,10 +36,13 @@ getHoverInfoFromState state (Just path) l c = do
case def of
BoundTerm t defloc -> do
Just $
(prettyText qn <> " : " <> prettyText t <> "\n\n")
<> ("**Definition: " <> T.pack (locStr (srclocOf defloc)) <> "**")
bound ->
Just $ "Definition: " <> T.pack (locStr (boundLoc bound))
(prettyText qn <> " : " <> prettyText t)
<> if isBuiltinLoc defloc
then mempty
else "\n\n**Definition: " <> T.pack (locStr (srclocOf defloc)) <> "**"
bound
| isBuiltinLoc (boundLoc bound) -> Just "Builtin definition."
| otherwise -> Just $ "Definition: " <> T.pack (locStr (boundLoc bound))
getHoverInfoFromState _ _ _ _ = Nothing

findDefinitionRange :: State -> Maybe FilePath -> Int -> Int -> Maybe Location
Expand All @@ -49,7 +52,7 @@ findDefinitionRange state (Just path) l c = do
AtName _qn (Just bound) _loc <- queryAtPos state $ Pos path l c 0
let loc = boundLoc bound
Loc (Pos file_path _ _ _) _ = loc
if isBuiltin file_path
if isBuiltinLoc loc
then Nothing
else Just $ Location (filePathToUri file_path) (rangeFromLoc loc)
findDefinitionRange _ _ _ _ = Nothing
Expand Down
13 changes: 12 additions & 1 deletion src/Language/Futhark/Prop.hs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ module Language.Futhark.Prop
Intrinsic (..),
intrinsics,
isBuiltin,
isBuiltinLoc,
maxIntrinsicTag,
namesToPrimTypes,
qualName,
Expand Down Expand Up @@ -113,6 +114,7 @@ import Data.Bitraversable (bitraverse)
import Data.Char
import Data.Foldable
import Data.List (genericLength, isPrefixOf, sortOn)
import Data.Loc (Loc (..), posFile)
import qualified Data.Map.Strict as M
import Data.Maybe
import Data.Ord
Expand Down Expand Up @@ -1259,9 +1261,18 @@ intrinsics =
tupInt64 x =
tupleRecord $ replicate x $ Scalar $ Prim $ Signed Int64

isBuiltin :: String -> Bool
-- | Is this file part of the built-in prelude?
isBuiltin :: FilePath -> Bool
isBuiltin = ("/prelude/" `isPrefixOf`)

-- | Is the position of this thing builtin as per 'isBuiltin'? Things
-- without location are considered not built-in.
isBuiltinLoc :: Located a => a -> Bool
isBuiltinLoc x =
case locOf x of
NoLoc -> False
Loc pos _ -> isBuiltin $ posFile pos

-- | The largest tag used by an intrinsic - this can be used to
-- determine whether a 'VName' refers to an intrinsic or a user-defined name.
maxIntrinsicTag :: Int
Expand Down

5 comments on commit 919d686

@haoranpb
Copy link
Collaborator

@haoranpb haoranpb commented on 919d686 Apr 14, 2022

Choose a reason for hiding this comment

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

Can we keep the buildin location? I'm building a middleware on the client-side to add links to documentation when it's part of prelude, and the logic relies on the /prelude/*.fut string

diku-dk/futhark-vscode@6b159ba

@athas
Copy link
Member Author

@athas athas commented on 919d686 Apr 14, 2022

Choose a reason for hiding this comment

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

Sure, if you can use it for something. But I must admit I think it's useless noise to show the location on hover.

@haoranpb
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, but I hope the link to the documentation site will make it useful, convenient to navigate to the corresponding page
Screen Shot 2022-04-14 at 12 56 48

@athas
Copy link
Member Author

@athas athas commented on 919d686 Apr 14, 2022

Choose a reason for hiding this comment

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

The "Definition" part doesn't have to be there, though. Extracting this information through parsing a string seems like the wrong approach.

@haoranpb
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. I can construct the link to the documentation site in this line, no need for the client-side to get involved

| isBuiltinLoc (boundLoc bound) -> Just "Builtin definition."

Please sign in to comment.