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

RFC: Amended Require Syntax and Resolution Semantics #56

Merged
merged 13 commits into from
Sep 25, 2024

Conversation

vrn-sn
Copy link
Contributor

@vrn-sn vrn-sn commented Sep 11, 2024

Rendered

We need to finalize a syntax for aliases in require expressions and determine intuitive resolution semantics as we prepare to build a package management system.

Specifically, this proposal finalizes a syntax for require expressions, removes the paths configuration variable, and throws errors when requiring paths that cannot be disambiguated to a script.

@vrn-sn vrn-sn self-assigned this Sep 11, 2024
Relative path parenting must already be done explicitly and is not up for discussion.
@dphfox
Copy link

dphfox commented Sep 11, 2024

I'm torn personally. Originally I imagined this using @ personally (which I'd've been fine with - it's load bearing syntax!) but now that you mention it, it's normally nicer to explicitly mark relative requires. But of course, that's not how existing string requires work. Hmm.

I'd say prioritise backwards compatibility with Lua (we want to give everyone the option to integrate with the wider Lua ecosystem at some point, right?). I don't think @ is that bad in the slightest, even though it's lamentable that people use relative paths as they do.

@alexmccord
Copy link
Contributor

alexmccord commented Sep 11, 2024

It must be mentioned that I am of the opinion that having a require resolution order is a workaround for poor design, so I'm very firmly in the camp of 1c. A softer version I'm okay with, given sufficient persuasion, is 1a. All other designs are no-go.

@bradsharp
Copy link
Contributor

bradsharp commented Sep 12, 2024

I'd say prioritise backwards compatibility with Lua (we want to give everyone the option to integrate with the wider Lua ecosystem at some point, right?). I don't think @ is that bad in the slightest, even though it's lamentable that people use relative paths as they do.

Lua uses a different convention entirely with . as the separator. Additionally, a number of Lua libraries are written in C so I'm not sure how practical that is (but it's something we should definitely be thinking about).

Regarding backwards compatibility with existing Luau code, it seems there are two conventions used in different codebases. In the past Luau was more restrictive, requiring @ and ./ but at some point that changed to remove the ./ requirement. I think the question here is whether that should have happened, and it would be better to answer that once and for good now than attempt to change this in the future.

@bradsharp
Copy link
Contributor

bradsharp commented Sep 12, 2024

My preference is for 1b, relative requires must start with ./ which was the behavior proposed in #969.

  • Some applications treat ./foo and foo differently when there's a different file directory and working directory
  • Any path starting with . is always going to be a relative file path
  • Implicit resolution order can catch you off guard (though I don't hate it if there are explicit prefixes I can use)
  • Prefixing all my external imports with @ makes them look a bit ugly

Unfortunately, this also breaks some existing Luau code but I'm hoping since we're discussing this early enough that the impact is minimal.

@dphblox
Copy link

dphblox commented Sep 12, 2024

Got it. I was under the impression Lua used normal fs paths, thanks for the correction :)

I'll change my answer then to reflect that relaxed constraint:

It must be mentioned that I am of the opinion that having a require resolution order is a workaround for poor design

Firm agree. In general, it should be clear and unambiguous what you're trying to require, at the point of the require. I don't want to have to go hunting dependencies down.

In the past Luau was more restrictive, requiring @ and ./ but at some point that changed to remove the ./ requirement. I think the question here is whether that should have happened

Yeah, I personally think ./ should be required to make the intent clear. I don't usually want to do relative requires when importing dependencies; it's normally just shorthand for requiring modules inside the current directory that implement private things or otherwise don't have a concise global presence. I would not want my intention to import a library to be misinterpreted just because my project isn't set up correctly, nor would I be happy debugging someone else's project that can't find the configuration somehow, and only being left with non-actionable errors about relative file paths being missing.

At that point, I suppose you can go with or without @ - since it's clearly and unambiguously referring to an alias - so I could live with either choice.

@vrn-sn
Copy link
Contributor Author

vrn-sn commented Sep 12, 2024

I want to wait for @vegorov-rbx and @aatxe to express their preferences on the designs, but it does seem like we're leaning in the direction of the explicit resolution methods (1a, 1b, 1c), rather than the implicit ones (3a, 3b). The "almost-explicit" (2) is really just a less performant version of (1c) that tries to minimize breaking backwards compatibility—if we don't care about this, then I think (1c) is strictly better than (2).

One confusing case with (2) that I can imagine is a developer's .luaurc file (perhaps globally defined) defining conflicting aliases with an external Luau package's relative requires. (Although, maybe that would be the package developer's responsibility to avoid by using the explicit ./ prefix.) The point remains that (2) is not entirely environment-agnostic when it comes to unprefixed relative paths, so it could lead to some unexpected behavior.

I'm a fan of (1c). Despite it being a bit uglier, I think it's the responsibility of the language to be unambiguous, and the responsibility of an LSP to make it easy for the developer (i.e. autocomplete). With (1c), autocomplete can have a clear indication of when to branch to a new set of possibilities.

-- Autocomplete options: "@", "./", "../"
require("
-- Autocomplete options: all available aliases
require("@
-- Autocomplete options: all available modules in requirer's directory
require("./

In contrast, (1a) and (1b) are slightly more confusing in terms of autocomplete options.

-- Autocomplete options: need to show valid string path components and all three prefixes ("@", "./", "../")
-- "Valid string path components" means local modules for (1a) and aliases for (1b)
require("

@aviralg
Copy link
Contributor

aviralg commented Sep 12, 2024

1A, 1B, and 1C all seem fine to me. I think there is value in 1C since it is explicit and enables better autocomplete support.
Personally, I don't find prefixing paths with either @ or ./, ugly.
I am interested in knowing if compatibility with DarkLua and Lune matters to our users.

@aatxe
Copy link
Contributor

aatxe commented Sep 16, 2024

All of the (1) options are good with me, with a preference towards 1B or 1C. I'm okay with the idea that libraries are the default (and therefore aliases are the default), hence 1B, and 1C is more-or-less the same to me, just with library names being under @. The most important consideration for me is not having implicit resolution rules that people have to learn and internalize (3), and not having strange quirky behaviors depending on the environment (2), and that's why the (1) options are the best in my book.

@dphblox
Copy link

dphblox commented Sep 16, 2024

Judging by the vibes in these replies, I guess the question is just whether we think relative paths are more frequently used than aliases. We could probably try and get some metrics for this if we wanted data.

Does anyone have strong feelings about relative paths without a prefix?

@vrn-sn
Copy link
Contributor Author

vrn-sn commented Sep 16, 2024

I guess the question is just whether we think relative paths are more frequently used than aliases.

We also care about how we want to design our require syntax as we think about a package manager. If backwards compatibility isn't a high priority, then it might not matter if relative requires are used more frequently right now. For example, if our ultimate goal is to support modular workflows that involve aliased imports (facilitated by a package manager), then it may make more sense to lean away from (1a), which simplifies the syntax for relative paths instead.

My point is that our goals for future usage patterns may matter more than current usage patterns, depending on how highly we prioritize backwards compatibility.

@dphblox
Copy link

dphblox commented Sep 16, 2024

Fair point, I hadn't considered that.

I guess something else we should remember is that we can always "reserve" the unprefixed variant and make this choice when we have more of an idea of how package management is going to affect all of this, and use both @ and ./ for now.

Though personally I'm already leaning towards mandating ./ for relative paths, since I agree that modular workflows could feasibly take over quite significantly once we have a proper package management system in place. Most of the codebases I've worked on have more dependencies between packages than within packages (because they tend to build on each other, e.g. building on Roact, or Fusion, or various util modules...), and I would wager that's going to increase over time once we have official package management.

@Dekkonot
Copy link
Contributor

I think option 3 is unacceptable, but that thankfully seems to be a very popular opinion. In the same vein, 2 is just a worse version of 1c. If you make something optional, it's probably not going to be used by the bulk of users, so we should just bite the bullet and make it required.

I don't super care for requiring a prefix for relative requires, because I like being able to go require("sibling"). It's unintuitive to me at least to require it to be require("./sibling"). That's really my only complaint about option 1c though, so I'm favoring that one. If it turns out to be a problem, it can always be relaxed because it effectively reserves the unprefixed require syntax.

@vegorov-rbx
Copy link
Contributor

Took a look at our internal Luau codebase.
It's a bit hard to categorize, but here are some results:

20090 total require statements.

  • 8358 refer to an external package folder (would be explicit @).
  • 6584 refer to an internal module coming described from the root (would be either an @ alias to the root or the 'paths' will contain root path, so not a ./)
  • 2824 refer to an internal module not from root (would be a specific alias @)
  • 1023 refer to a sibling (would be explicit ./)
  • 808 refer to an outer directory using FindFirstAncestor, better to rewrite into an @ alias instead
  • 60 refer to an outer directory (../)
  • 433 remaining look mostly like something which could use an alias

@vrn-sn
Copy link
Contributor Author

vrn-sn commented Sep 16, 2024

I've also moved approaches (2), (3a), and (3b) to alternatives. It seems like we're in favor of the explicit options and now just need to choose from (1a), (1b), and (1c).

@dphblox and @Dekkonot both mentioned a great point about how (1c) effectively reserves unprefixed paths, so I just added a small section to (1c) about it being forwards-compatible with both (1a) and (1b).

@dphfox
Copy link

dphfox commented Sep 17, 2024

Agreed on path resolution being weird. I really don't think we should be using path resolution at all. It strikes me as exactly the kind of implicit behaviour that people here are taking issue with - better to explicitly alias?

As for ./ being hard to write in requires, what does the autocomplete look like today? It could be that LSP support isn't where it needs to be yet.

@witchiestwitchery
Copy link
Contributor

Agreed on path resolution being weird. I really don't think we should be using path resolution at all. It strikes me as exactly the kind of implicit behaviour that people here are taking issue with - better to explicitly alias?

As for ./ being hard to write in requires, what does the autocomplete look like today? It could be that LSP support isn't where it needs to be yet.

Autocomplete doesnt exist, I mean it'll warn you for providing an invalid path but that's it.

@vrn-sn
Copy link
Contributor Author

vrn-sn commented Sep 17, 2024

Is it worth noting how these different semantics work with the paths aspect that was mentioned in the original RFC (https://github.com/luau-lang/rfcs/blob/master/docs/require-by-string-aliases.md#paths)? As it doesn't look like it's been considered here, apart from a brief mention in option (2).

When we started discussing this internally, the options we were considering didn't seem like they'd impact the paths array, which is why it didn't make it into this RFC. You're right though, we will need to discuss that impact as well.

I agree that ./ should only be relative to the requiring file. To recap, we currently have:

  • (1a) unprefixed paths are treated as relative
  • (1b) unprefixed paths are treated as aliased
  • (1c) unprefixed paths are forbidden altogether

With (1b) and (1c), it's unclear what syntax could support the paths array fallback search. For (1c), we could allow unprefixed paths but only resolve them relative to paths, but this approach wouldn't work for (1b) since unprefixed paths already have a designated purpose. We could also add a new prefix for checking paths like require("$mydependency"), which works for both (1b) and (1c), but it's clunky and might confuse the purposes of paths and aliases.

As far as compatibility with paths, (1a) is the best option, as we could continue using paths as a fallback for unprefixed relative requires.

Agreed on path resolution being weird. I really don't think we should be using path resolution at all. It strikes me as exactly the kind of implicit behaviour that people here are taking issue with - better to explicitly alias?

I'm not sure we want to completely get rid of paths behavior altogether. It does provide convenience, and it's not completely implicit in the way that options (3a) and (3b) are in this proposal (i.e. the resolution order is explicitly specified in .luaurc). What do you all think?

@dphblox
Copy link

dphblox commented Sep 17, 2024

I suppose to be clearer about why I don't like it, it's because I don't like it when projects share packages (e.g. having one folder in the root of C:/ or wherever, where all installed packages like Roact etc live). It leads to all sorts of problems, especially when two projects need a package of the same name, but different versions (or worse - two unrelated packages with a name collision). I feel like paths doesn't serve much use within a single repository (just use aliases), and only becomes useful if you're trying to implement that sort of shared system.

I suppose all of this'll end up being the responsibility for whatever package management system we land on though, so maybe it's OK to have? I'm not sure.

It feels a little confused to me, I guess? Maybe I don't understand what paths is actually for.

@JohnnyMorganz
Copy link

Agreed with all the points raised by @dphblox. Personally I'm not sold on the use cases of paths, they don't seem that strong. and I also feel like it reverts to that implicit ordering that people raised as a concern.

Maybe it's not that implicit, but the ordering still matters. For ./ paths it is quite direct what file it points to (1:1 mapping), similarly for aliases you can resolve a 1:1 mapping, but for paths you have to linearly search through all the defined paths to figure out what file it's pointing too (I care about this more from the perspective of a human reading it statically rather than a computer perspective - computers are fast and LSP can provide go to definition, but those tools aren't always available during a code review). Ordering of the paths in the array can affect resolution of files, when there are two files with the same (sub-)path across different paths. Would one need to go back to the .luaurc, visit one by one each directory path specified in order to then figure out where a require is actually pointing to?

The behaviour of paths can also be replicated using aliases instead (e.g @dep/ instead of a dependencies folder in a paths array). Sure, couple extra characters, but I think the explicitness is more valuable.

I would argue for actually removing paths support completely, but maybe that's too strong / it's too late.

@JohnnyMorganz
Copy link

JohnnyMorganz commented Sep 17, 2024

Not completely related to this RFC, so feel free to ignore, but also worth considering how paths would interact with a future package manager?

I cannot see a world where paths would be usable when you want to have portable packages, apart from if the only paths listed there are within the scope of the package itself (or its pointing to the Packages/ or node_modules/-like folder generated by the package manager). But to be fair, the same restriction would probably apply to aliases too. But I think those types of aliases are still useful, whilst "only-within-package-scope" paths don't seem like it to me.

I haven't thought about that too deeply though (and I may be biased against paths at this point :P) so maybe you folks have already considered this.

@vrn-sn
Copy link
Contributor Author

vrn-sn commented Sep 17, 2024

Discussed with @aatxe a bit just now.

The initial motivation for paths was likely to support something similar to Lua's LUA_PATH. At this point, we've diverged enough from Lua that I can see the argument for removing paths altogether. Parroting the good points made above, I believe any use case that involves paths can use aliases instead, and this also has the benefit of being more explicit.

Our package manager implementation could utilize paths, but after discussion, we'll be fine solely using aliases instead to point to dependencies. Also, from looking at the analysis that @vegorov-rbx posted, the only case in which the paths variable might be appropriate would work fine implemented as an alias:

6584 refer to an internal module coming described from the root (would be either an @ alias to the root or the 'paths' will contain root path, so not a ./)

I'll refactor this RFC to propose removing the paths variable altogether. I also want to start to move toward one of the three approaches (1a, 1b, 1c), assuming paths no longer exists.

@vrn-sn
Copy link
Contributor Author

vrn-sn commented Sep 18, 2024

To help this proposal move forward now, I think the best course of action is to formally propose (1c) and move (1a) and (1b) to alternatives. Approach (1c) is the most restrictive of the three options and prevents us from making a decision about unprefixed paths before we know what the most common use case will be (say, in the context of a package manager).

Despite being the most restrictive, it feels like a good compromise to me because of this forward compatibility with both (1a) and (1b).

I get your point here @Dekkonot:

As predicted, including the ./ in requires is incredibly cumbersome, especially for symbols. I ended up writing .modules/FileName more often than ./modules/FileName. It may be a skill issue on my part, but I'm really not in love with it.

I think I might have to change my stance to 1a just based on my own experience. The harm to intellisense isn't worth the hassle IMO.

Still, I'm a bit wary about going in the direction of (1a), building a package manager, and then getting similar complaints about the mandated @ prefix (assuming aliased modules will be more commonly used). If anything, workflows in the context of package management seem to fit better with (1b), but I think it's too early to make a definite decision right now.

I think the best option is to choose the approach that gives us the most flexibility with unprefixed paths. Unless someone has strong opposing thoughts on this, I think we should move forward with (1c).

@vrn-sn vrn-sn changed the title RFC: Amended Alias Syntax and Resolution Semantics RFC: Amended Require Syntax and Resolution Semantics Sep 19, 2024
@vrn-sn
Copy link
Contributor Author

vrn-sn commented Sep 19, 2024

Reworked the RFC: would appreciate PR review now. I'll plan on merging in these changes in a week if there is no opposition.

@alexmccord
Copy link
Contributor

alexmccord commented Sep 19, 2024

One last thing I'm not currently seeing in the RFC. If we still support both .luau and .lua extensions, then there is still a require resolution ordering and that has some number of possible variants.

Given this scenario:

src/
  mod/
    init.luau
  mod.luau
  mod.lua

If I have require("./src/mod"), what does this resolve to?

I would like to see us stop supporting .lua extension entirely, but this is probably too big of a change for the time being, and still doesn't resolve the issue with whether the require resolves to ./src/mod/init.luau and ./src/mod.luau.

@aatxe
Copy link
Contributor

aatxe commented Sep 19, 2024

Two sane behaviors IMO are: prefer luau to lua, or error about the conflict.

@Dekkonot
Copy link
Contributor

My preference would be an error in that situation, because I cannot imagine it being deliberate.

@vrn-sn
Copy link
Contributor Author

vrn-sn commented Sep 19, 2024

I've added a section about throwing an error in any case in which there's ambiguity, like @alexmccord's example:

Given this scenario:

src/
  mod/
    init.luau
  mod.luau
  mod.lua

If I have require("./src/mod"), what does this resolve to?

I think that's in line with making everything else explicit that we've discussed in this PR.

@dphfox
Copy link

dphfox commented Sep 20, 2024

On the topic of ambiguity, would this also error if multiple files shared the same name? This isn't possible on disk but is possible in the DataModel.

Right now when performing an ambiguous indexing operation in Roblox, it always has to succeed for backcompat reasons, but perhaps we could take this opportunity to explicitly disallow it for this new syntax, and for any path resolution style APIs going forward?

@vrn-sn
Copy link
Contributor Author

vrn-sn commented Sep 23, 2024

@dphfox and I discussed this internally:

Right now when performing an ambiguous indexing operation in Roblox, it always has to succeed for backcompat reasons, but perhaps we could take this opportunity to explicitly disallow it for this new syntax, and for any path resolution style APIs going forward?

We're going to leave Roblox-related details out of the scope of this RFC, but the goal is to keep things as compatible as possible. In Luau's implementation, I don't think we need to add an additional check for this, as the file system implicitly enforces it for us.

@Dekkonot
Copy link
Contributor

That's a fair attitude to take. It's an implementation detail that Roblox is a graph. Basically everywhere else Luau exists it shouldn't matter.

@andyfriesen andyfriesen merged commit 79c7227 into luau-lang:master Sep 25, 2024
@vrn-sn vrn-sn deleted the alias-resolution-rfc branch September 25, 2024 22:07
andyfriesen added a commit to luau-lang/luau that referenced this pull request Oct 4, 2024
# General Updates

* Fix some cases where documentation symbols would not be available when
mouseovering at certain positions in the code
* Scaffolding to help embedders have more control over how `typeof(x)`
refines types
* Refinements to require-by-string semantics. See
luau-lang/rfcs#56 for details.
* Fix for #1405

# New Solver

* Fix many crashes (thanks you for your bug reports!)
* Type functions can now call each other
* Type functions all evaluate in a single VM. This should improve
typechecking performance and reduce memory use.
* `export type function` is now forbidden and fails with a clear error
message
* Type functions that access locals in the surrounding environment are
now properly a parse error
* You can now use `:setindexer(types.never, types.never)` to delete an
indexer from a table type.

# Internal Contributors

Co-authored-by: Aaron Weiss <[email protected]>
Co-authored-by: Hunter Goldstein <[email protected]>
Co-authored-by: Varun Saini <[email protected]>
Co-authored-by: Vyacheslav Egorov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.