Limit definition search to provided relativeTo #3194

ChrisPenner · 2022-07-05T23:23:42Z

Overview

Names are really confusing, currently the 'show definition by name' endpoint constructs a names object relative to where it's supposed to be, but still includes 'fall-backs' for all external names. When you call to get a definition by name, the server returns all matches, and the frontend picks on and renders it.

Problems:

The current name-search isn't strictly scoped to the 'relativeTo', so sometimes the frontend renders a completely different definition than the one you asked for
In general names aren't optimal in rendered definitions

Fix

If we're provided a relativeTo, we now strictly limit any returned terms to names that actually exist within that namespace.
Limit searches for things like docs, and also hashes to ones strictly inside our perspective too.
Change most PPEs to the new "smarter" PPE design by Paul in Don't consider names outside the current namespace when pretty-printing, unless necessary #3172

Additional changes:

Make sorting out names quicker by using the recently added namespace column on the names index.

Old view of `trunk.Seq`

The names we're getting here are actually from a completely different definition than the one we asked for 😬

New view of `trunk.Seq` from `public.distributed`

Here we can see that we still get some less-desirable names here, there are multiple Seq's in the current scope, collab.Seq and trunk.Seq are the same hash, but collab.Seq is preferred simply because it's earlier alphabetically.

I have an idea for fixing this problem too, but I'll split that into a separate PR if I can figure it out.

New view of `Seq` from `public.distributed.trunk`

Now that we're within the perspective of trunk all of the names are properly minimally prefixed and what we expect!

unison-core/src/Unison/Names.hs

ChrisPenner · 2022-07-06T17:44:12Z

unison-core/src/Unison/NamesWithHistory.hs

@@ -274,7 +274,7 @@ suffixedTermName :: Int -> Referent -> NamesWithHistory -> [HQ'.HashQualified Na
                  isHQ'd = R.manyDom fqn rel -- it is conflicted
                  hq n = HQ'.take length (hq' n r)
                  hqn = if isHQ'd then hq n' else HQ'.fromName n'
-               in (isHQ'd, Name.countSegments fqn, Name.isAbsolute n', hqn)
+               in (isHQ'd, Name.countSegments n', Name.isAbsolute n', hqn)


Prefer shortest name by the abbreviated name, rather than the fully qualified name.

ChrisPenner · 2022-07-06T17:44:41Z

unison-share-api/src/Unison/Server/Backend.hs

@@ -322,13 +322,11 @@ findShallowReadmeInBranchAndRender ::
  Width ->
  Rt.Runtime Symbol ->
  Codebase IO Symbol Ann ->
-  NamesWithHistory ->
+  PPE.PrettyPrintEnvDecl ->


This function only uses a PPE

ChrisPenner · 2022-07-06T17:46:14Z

unison-share-api/src/Unison/Server/Backend.hs

+        else do
+          flip prettyAndParseNamesForBranch (AllNames path) <$> resolveCausalHash (Just bh) codebase
+
+  let localPPE = PPE.fromNamesDecl hashLen (NamesWithHistory.fromCurrentNames localNames)


Use the trick from #3172 to improve names chosen in PPE

ChrisPenner · 2022-07-06T21:13:02Z

codebase2/codebase-sqlite/U/Codebase/Sqlite/Operations.hs

-  termNames <- Q.rootTermNames
-  typeNames <- Q.rootTypeNames
-  pure (fmap (bimap s2cTextReferent (fmap s2cConstructorType)) <$> termNames, fmap s2cTextReference <$> typeNames)
+rootNamesByPath ::


Now that I have the namespace in sqlite it's easy to partition names in sqlite which is likely faster than doing it in Haskell.

ChrisPenner · 2022-07-06T21:14:58Z

unison-share-api/src/Unison/Server/Backend.hs

-        PPE.fromNamesDecl hqLength (NamesWithHistory.fromCurrentNames printNames)
-
-      width =
+  (_parseNames, _printNames, localNamesOnly, ppe) <- scopedNamesForBranchHash codebase root path


The biggest change in this function is that we previously used "prettyNames" for our initial term search which would erroneously fall-back to global names even though we're explicitly searching within a namespace.

Now any searches for scope-local things use localNamesOnly explicitly, and in cases where fallback is desired we use a smart PPE which does suffixification and fall-back correctly.

ChrisPenner · 2022-07-06T23:54:49Z

unison-core/src/Unison/Names.hs

+-- E.g.
+-- ["base.List.map", "base.Bag.map"] -> ["List.map", "Bag.map"]
+-- ["base.List.filter", "base.List.map"] -> ["filter", "map"]
+minimalUniqueSuffix :: Names -> Names


I initially thought I would need this, but it's easier to handle within the PPE layer so this is currently unused.

I'd prefer to just leave it here since it may be useful in the future, but happy to remove it if it seems clutter-full

ChrisPenner · 2022-07-07T00:01:10Z

unison-src/transcripts/api-getDefinition.output.md

@@ -312,99 +312,11 @@ GET /api/getDefinition?names=%23qkhkl0n238&relativeTo=nested
    },
    "typeDefinitions": {}
 }
--  Should find definitions by hash, using global names if no names in specified path.
+--  Should filter out any definitions which aren't in the provided namespace even if the hash matches.


I believe these old semantics were sub-optimal anyways, if you specifically search for a hash within a specific namespace, I think you should only get hashes that are in that namespace. If you want to do a global search for a hash you can just search at the namespace root.

I need another PR which PREFERS local names by hash, but will fall back to global names, biased towards the current perspective.

This is so you can click on external names in a pretty-printed definition.

pchiusano · 2022-07-07T00:11:19Z

Awesome @ChrisPenner

exactly what I’d hoped for.

I’ll let other folks weigh in on the code changes, but the new behavior looks great!

pchiusano · 2022-07-09T12:55:09Z

@ChrisPenner just wanted to ask you about https://share-next.unison-lang.org/users/unison/code/latest/namespaces/public/base/main/;/terms/abilities/Random/shuffle

Notice that Random.splitmix is getting the name latest.Random.splitmix (same number of segments as main.Random.splitmix, but l comes before m)

Random.splitmix (in main, the current perspective) is the same as latest.Random.splitmix (not even in the current perspective)

I'm not sure if your other names-related PR addresses this (or if this one does but it's just not deployed yet), but we should never show a name for a definition from outside the current perspective if that definition has a name within the current perspective.

aryairani · 2022-07-10T16:10:37Z

unison-share-api/src/Unison/Server/Backend.hs

+-- - 'local' includes ONLY the names within the provided path
+-- - 'ppe' is a ppe which searches for a name within the path first, but falls back to a global name search.
+--     The 'suffixified' component of this ppe will search for the shortest unambiguous suffix within the scope in which the name is found (local, falling back to global)
+scopedNamesForBranchHash :: forall m v a. Monad m => Codebase m v a -> Maybe (Branch.CausalHash) -> Path -> Backend m (Names, Names, Names, PPE.PrettyPrintEnvDecl)


Do we use the first two return values anywhere?

I think we still need to compute them to make the PPE, but no I don't think I need to return them, I can change that 👍🏼

ChrisPenner · 2022-07-11T16:23:15Z

@pchiusano

Random.splitmix (in main, the current perspective) is the same as latest.Random.splitmix (not even in the current perspective)

Are you sure? I'm seeing different hashes for these:

I think the base.main doc just refers to the splitmix from base.latest by accident.

I do think I need to re-evaluate how we filter out 'hash-only' queries though 🤔

pchiusano · 2022-07-11T16:38:57Z

Good call. You're right.

I just tracked down why these are different, and it looks like main of base replaced the implementation of toBytesLittleEndian to use a builtin.

I'll fixup the docs in base.

Update: this is done and looks good now: https://share-next.unison-lang.org/users/unison/code/latest/namespaces/public/base/main/;/terms/abilities/Random/shuffle

mitchellwrosen · 2022-07-12T14:51:49Z

codebase2/codebase-sqlite/U/Codebase/Sqlite/Queries.hs

+rootTermNamesByPath mayNamespace = do
+  let (namespace, subnamespace) = case mayNamespace of
+        Nothing -> ("", "*")
+        Just namespace -> (namespace, globEscape namespace <> ".*")


I wonder if we should escape namespace too, in case we go back to allowing things like Int.*.doc? In fact, I wonder if we should escape . characters in name segments for that reason, too.

namespace isn't being passed to a GLOB, it's used in an equality comparison, so there's no need to escape it, or did you mean something else?

mitchellwrosen · 2022-07-12T15:00:41Z

parser-typechecker/src/Unison/Codebase/SqliteCodebase/Operations.hs

-        Nothing -> (mempty, [(n, ref)])
-        Just stripped -> ([(Name.makeRelative stripped, ref)], mempty)
+        Nothing -> error $ "Expected name to be in namespace" <> show (n, reverse reversedPathSegments)
+        Just stripped -> (Name.makeRelative stripped, ref)


I'm wondering if the Name.makeRelative is necessary here - aren't all of these names relative, even those that we put in absoluteExternalNames? Or - if they get converted to absolute names at some point - where does that happen?

To be honest I think we have a lot of unnecessary work going on with names but it's extremely difficult to get my head around it and make too many changes at once without breaking everything and not knowing why haha.

I think what I envision is that actually names objects should probably just have all absolutely qualified names, then the PPE should make them relative and/or suffixify them when it's time to display them.

mitchellwrosen

Approving with the disclaimer that I don't totally understand what's going on, even after the call 😅

My understanding is really just: things were very wrong before, now after a lot of changes, they are better, but not optimal.

ChrisPenner added 2 commits July 5, 2022 11:06

Add combinator to compute minimal unique suffixes.

9623359

WIP - More strict definition by name

5ea90e7

ChrisPenner changed the title ~~Cp/abbreviate names~~ Limit definition search to provided relativeTo Jul 5, 2022

ChrisPenner added 2 commits July 6, 2022 11:26

Prefer using PPE over names

c0efe66

Docs

783d988

ChrisPenner commented Jul 6, 2022

View reviewed changes

unison-core/src/Unison/Names.hs Outdated Show resolved Hide resolved

ChrisPenner commented Jul 6, 2022

View reviewed changes

ChrisPenner added 3 commits July 6, 2022 11:46

Remove traces

715d624

Filter out hashes which are outside of perspective for definition apis

b1e4558

Move names filtering up to API level

c8a0ab2

ChrisPenner force-pushed the cp/abbreviate-names branch from e57861f to c8a0ab2 Compare July 6, 2022 21:10

ChrisPenner commented Jul 6, 2022

View reviewed changes

Flip name selection priority

c8a6802

ChrisPenner self-assigned this Jul 6, 2022

ChrisPenner marked this pull request as ready for review July 6, 2022 23:53

ChrisPenner commented Jul 6, 2022

View reviewed changes

ChrisPenner requested review from aryairani, mitchellwrosen and tstat July 6, 2022 23:54

ChrisPenner commented Jul 7, 2022

View reviewed changes

pchiusano approved these changes Jul 7, 2022

View reviewed changes

ChrisPenner mentioned this pull request Jul 7, 2022

Bias PPE's towards definition being printed #3202

Closed

pchiusano added this to the M4 milestone Jul 8, 2022

aryairani reviewed Jul 10, 2022

View reviewed changes

ChrisPenner added 2 commits July 11, 2022 14:33

Remove unused minimalUniqueSuffix for Names

2804022

Remove unused names from scoped names

de778d4

ChrisPenner force-pushed the cp/abbreviate-names branch from cb71ded to de778d4 Compare July 11, 2022 21:08

Merge branch 'trunk' into cp/abbreviate-names

bad5acb

mitchellwrosen reviewed Jul 12, 2022

View reviewed changes

mitchellwrosen approved these changes Jul 12, 2022

View reviewed changes

ChrisPenner merged commit 6ab3865 into trunk Jul 12, 2022

ChrisPenner deleted the cp/abbreviate-names branch July 12, 2022 15:46

aryairani mentioned this pull request Jul 14, 2022

M4 Release Notes (DRAFT) #3209

Closed

ChrisPenner mentioned this pull request Jul 20, 2022

Bias PPE towards term being printed #3260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit definition search to provided relativeTo #3194

Limit definition search to provided relativeTo #3194

ChrisPenner commented Jul 5, 2022 •

edited

ChrisPenner Jul 6, 2022

ChrisPenner Jul 6, 2022

ChrisPenner Jul 6, 2022

ChrisPenner Jul 6, 2022

ChrisPenner Jul 6, 2022

ChrisPenner Jul 6, 2022

ChrisPenner Jul 7, 2022

ChrisPenner Jul 11, 2022

pchiusano commented Jul 7, 2022

pchiusano commented Jul 9, 2022

aryairani Jul 10, 2022 •

edited

ChrisPenner Jul 11, 2022 •

edited

ChrisPenner commented Jul 11, 2022

pchiusano commented Jul 11, 2022 •

edited

mitchellwrosen Jul 12, 2022

ChrisPenner Jul 12, 2022 •

edited

mitchellwrosen Jul 12, 2022

ChrisPenner Jul 12, 2022

mitchellwrosen left a comment

Limit definition search to provided relativeTo #3194

Limit definition search to provided relativeTo #3194

Conversation

ChrisPenner commented Jul 5, 2022 • edited

Overview

Fix

Old view of trunk.Seq

New view of trunk.Seq from public.distributed

New view of Seq from public.distributed.trunk

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchiusano commented Jul 7, 2022

pchiusano commented Jul 9, 2022

aryairani Jul 10, 2022 • edited

Choose a reason for hiding this comment

ChrisPenner Jul 11, 2022 • edited

Choose a reason for hiding this comment

ChrisPenner commented Jul 11, 2022

pchiusano commented Jul 11, 2022 • edited

Choose a reason for hiding this comment

ChrisPenner Jul 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchellwrosen left a comment

Choose a reason for hiding this comment

ChrisPenner commented Jul 5, 2022 •

edited

Old view of `trunk.Seq`

New view of `trunk.Seq` from `public.distributed`

New view of `Seq` from `public.distributed.trunk`

aryairani Jul 10, 2022 •

edited

ChrisPenner Jul 11, 2022 •

edited

pchiusano commented Jul 11, 2022 •

edited

ChrisPenner Jul 12, 2022 •

edited