unisonweb / unison

A friendly programming language from the future
https://unison-lang.org
Other
5.81k stars 271 forks source link

An Audit of Names and PPEs #3256

Open ChrisPenner opened 2 years ago

ChrisPenner commented 2 years ago

Names & PPEs

Unison's unique design choices also present unique challenges.

The AST doesn't store names for external definitions (since these names can change any time), so when parsing and printing we need to resolve names dynamically.

Glossary:

Where do we use Pretty Printers?

Where do we use Names?

How do we construct Names

There's often a distinction between pretty names and parse names.

Pretty names usually consists of relativized versions of all names in the current scope, as well as absolute names for everything outside of the current scope. Sometimes however it may include BOTH relativized and absolute names for things in the current scope.

Parse names consist of relative names within the provided scope, it may optionally include absolute versions of ALL names, including those in the current scope.

Relative vs Absolute

One source of confusion is that the contents of Names objects is often a mix of Relative and Absolute names, there's often little rhyme or reason behind the choice, and it's tricky to keep track of.

This makes it more difficult to do things like restrict a set of names to a given path, or to prioritize names close to a given location, since one needs to handle any combination of relative/absolute names.

When names are constructed directly from a unison file the names are, in some sense, neither absolute nor relative since they don't yet exist in the codebase.

Picking good names

Factors which affect goodness:

Due to the fact that "good" changes depending on the use-case it can be helpful to allow callers to customize the prioritization of their PPE on the fly.

Issues with the current system

Questions

Proposed Changes

data PrettyPrintEnv = PrettyPrintEnv
  { -- names for terms, constructors, and requests
    termNames :: Maybe Path -> Suffixify -> [Name] -> Referent -> [(HQ'.HashQualified Name, HQ'.HashQualified Name)],
    -- names for types
    typeNames :: Maybe Path -> Suffixify -> [Name] -> Reference -> [(HQ'.HashQualified Name, HQ'.HashQualified Name)],
    -- allows adjusting a pretty-printer to a specific perspective. Names within this perspective will be made relative.
    perspective :: Maybe Path,
    -- allows biasing returned names towards specific locations. 
    -- Names are automatically biased towards the current perspective if one is provided.
    biases :: [Name],
    -- Whether to shorten names to a minimal unambiguous suffix (within the current perspective) or not
    suffixify :: Suffixify
  }

It now returns a list of names in priority order, including the absolute name and the pretty, possibly relativized, possibly suffixified name

This supports a future change to a monadic pretty-printer, since a monadic version would likely need all parameters to be provided on each lookup so it can be as efficient as possible and not do any extra work.

It also allows altering a given PPE's parameters without needing to rebuild the whole thing from scratch. This means we can always share a single, global PPE and just alter its parameters for each pretty-printing task, saving us the work of re-building every time.

We can be careful to make PPE builders take advantage of currying to get as much sharing as possible and avoid re-computing the entire PPE when changing parameters. E.g. changing the biases requires only re-sorting the returned list, we don't need to re-suffixify or re-relativize or anything.

aryairani commented 2 years ago

The proposed changes sound good to me, though we should review the sample proposed PPE type to decide if it's trying to do too much.

aryairani commented 2 years ago

Generally we should avoid conflicted names, except when we're printing TODOs, in which case conflicted names are exactly what we want.

Not sure we want to avoid conflicted names, it might be what you want even outside of TODOs?

mitchellwrosen commented 2 years ago

we end up needing to re-create giant Names and PPEs any time the perspective changes

Does this mean when we cd we pay this big cost?

aryairani commented 2 years ago

We sometimes use Names in places where we're effectively pretty-printing, but not always

Out of curiosity, where are the spots that this is the case?

aryairani commented 2 years ago

Do parse names and pretty-printers still need to include names outside of the current scope? Or is that explicitly discouraged now?

For parsing scratch files, we currently aren't including names outside of the current scope. But for parsing REPL commands, I would think we should. e.g. alias.term .foo.bar.baz baz. Does this depend on that? I could see accepting absolute names in parsing too (why not?). But not included in suffix-based resolution. And it's okay to not accept them in parsing scratch files until we know more.

For pretty-printing, if we encounter a hash that doesn't have a name in the current scope, what should we do? a) show a bare hash, b) show an absolute name? The hashes aren't useful information; so if we can reasonably provide some useful information instead, we should.

aryairani commented 2 years ago

I would recommend Bias instead of just [Name] for biasing.

aryairani commented 2 years ago

A question about the proposed PrettyPrintEnv type:

What does its lifecycle look like? We note that it includes fields for perspective, bias, and suffixify setting, but also includes functions that accept those as arguments, so how is it all meant to work?

ChrisPenner commented 2 years ago

Some more questions that have come up during reviews: