Summary:
Might as well, since we have the information to hand.

I audited all the other uses of Vec::new(). They either come from things which are unknowable, things which read an iterator (and thus are also unknowable) or from tuple/list * operator, where I'm too worried about overflow to try anything (and aren't all that common).

Reviewed By: milend

Differential Revision: D27616002

fbshipit-source-id: 6755480abfe7f1521a438ac7efbb139daadcc202

Summary: Also, slim down the instances slightly, as some weren't required.

Reviewed By: cjhopman

Differential Revision: D27508352

fbshipit-source-id: bc33d34d8df01ed429e0e07f1684374a3dba1d28

Summary: To mirror the one for ARef. Allows defining this arguments on mutable values more concisely and more safely.

Reviewed By: cjhopman

Differential Revision: D27399709

fbshipit-source-id: 3f4085198f41902401910c5b7012840dc4f62b44

Summary: Before it just panic'd if there were unsupported types. That's not good at all. Instead, make it use proper error messages.

Reviewed By: bobyangyf

Differential Revision: D27684593

fbshipit-source-id: f9d41ba1451f5c5dda3f027f4c8654c29cf0859a

Summary: The case of not a function was never being hit, so remove it. It would correspond to a value attached to a type, which isn't documented as being allowed in module_value (you have attributes for that instead).

Reviewed By: blackm00n

Differential Revision: D27704549

fbshipit-source-id: 9a5dc40349bbd48f29fbc7f532027c92a8a14ce7

Summary: Whether something `is_function()` or not is a bit ambiguous. The spec says it must be things of type "function" that have a hash even though they probably shouldn't, so use that as the approach.

Reviewed By: blackm00n

Differential Revision: D27704551

fbshipit-source-id: 02083c454f8151bf2f0ac7372d73b8473a7ae362

Summary: The describe method is a bit dubious anyway, but might as well remove the one remaining use of `is_function()`. This test will be basically as good.

Reviewed By: blackm00n

Differential Revision: D27704552

fbshipit-source-id: 135f1badde97c7df85bddd576e1bd24d341aa25b

Summary: No longer used. Was never particularly clear what it should do for things that were both callable and values.

Reviewed By: blackm00n

Differential Revision: D27704553

fbshipit-source-id: 16a695b49a612a68b90a6fe6b08445fcc44eaeba

Summary: Now I slightly more understand what this does and why, it's worth making it a proper enumeration.

Reviewed By: blackm00n

Differential Revision: D27704550

fbshipit-source-id: 6dcb103d952026f1bb00afa0a0120f921b834951

Summary: The distinction is subtle (or at least, might not be subtle, but I missed it). For dot expressions the standard talks about methods vs fields, which behave differently, so mirror that language in our definitions.

Reviewed By: blackm00n

Differential Revision: D27704548

fbshipit-source-id: c7c66f788e74391e4f76e987d674875e1e2923fd

Summary: Follows fbcode rust update to 1.51

Reviewed By: ndmitchell

Differential Revision: D27656199

fbshipit-source-id: 3091b8bd58e9736ca0f9c5ad7bddcec8bf2ad7a8

Summary: Useful to access the to_json method.

Reviewed By: milend

Differential Revision: D27684592

fbshipit-source-id: 6f35b84961a1dd430eef10fd5306ce76fd8743bf

Summary: In the latest Rust nightly, the character `'` gets rendered as `'` in a String, whereas it used to be `\'`. The Debug implementation is considered unstable, so these kind of things can happen. Instead, switch to using our json to string code in unlex, which is mostly the same, but our code so well defined what it does.

Reviewed By: bobyangyf

Differential Revision: D27748476

fbshipit-source-id: a45507986372103a1d78af2d26651f000ac00d57

Summary:
Force use of nightly toolchain so that a developer can `git clone`, then `cargo test` regardless of their default toolchain

Pull Request resolved: #18

Differential Revision: D27762896

Pulled By: ndmitchell

fbshipit-source-id: c923da8869712b79663e6a3bfffcc289ec79a08d

Summary: The details of this type aren't secret, there are no invariants, and it simplifies things.

Reviewed By: bobyangyf

Differential Revision: D27829800

fbshipit-source-id: ff36c3b3f213786540912f04c6d4b695ea9f58df

Summary: Useful for writing some functions that you can't write without it. See #19 for an example.

Differential Revision: D27879459

fbshipit-source-id: dbe9a0296c0ba8b75f0e414b18fe37632e63b2d4

Summary: No point creating a stack guard unless you are going to recurse.

Differential Revision: D27879458

fbshipit-source-id: 2c6c5b4b33f7114af0f03ebd6be1260cc64bc9f5

Summary: Coming up with this was a bit of a brain teaser, so keen to reuse that logic. You often want to use GlobalsStatic to set fields. If you are using them with get_members, you can define a function with #[starlark_attribute], but it allocates the result each time and is a bit verbose. This new function doesn't allocate and can be used compactly.

Differential Revision: D27829799

fbshipit-source-id: 98d4af917e43293d271618705d0768c3ec9f6942

Summary: If the input is [String], we can use the method directly on slice, so do that instead.

Reviewed By: blackm00n

Differential Revision: D27821258

fbshipit-source-id: b71eee7759c073bb61d8cc740c07f701c3d34bb1

Summary: They are copy, so should be Dupe too.

Reviewed By: bobyangyf

Differential Revision: D27884291

fbshipit-source-id: 0c3563ceddeade7488198c6ec08050154a96253c

Summary: We rely on having Dupe for NonZero types, which was only added in Gazebo 0.2.1.

Reviewed By: milend

Differential Revision: D27908132

fbshipit-source-id: 5b9025b533323f80bc536dfbe24f449b80cd76a2

Summary:
These match stdlib maps/sets and are important when a caller may only
be able to produce a key-equivalent and needs to get out the full key (for
maps) or value (for sets). It's especially useful for sets that are keyed by
some projection of the value.

Reviewed By: ndmitchell

Differential Revision: D27528499

fbshipit-source-id: fa29d97cfdd4e2c69dfe956a759f758f60c9642c

Summary: All StarlarkValue's had to be AnyLifetime because of the default blanked instance of AsStarlarkValue. That gave pretty weird error messages, by failing to resolve an instance you didn't care about rather than saying about the instance you did care about, and meant one place we had to repeat the AnyLifetime constraint. Making it more direct solves both those niggles.

Reviewed By: bobyangyf

Differential Revision: D27938636

fbshipit-source-id: 5d6dda0060afdc0a7bf868c2fa50dd1fad005923

Summary: An alternative to ValueOf that lets you grab mutable values.

Reviewed By: bobyangyf

Differential Revision: D27938635

fbshipit-source-id: a59a2c0007f561648db3e41cb8baa732f761b305

Summary:
This should die for three reasons:

* It's the only UnpackValue that meaningfully uses the Heap.
* It loses the error message when the value is already borrowed or is frozen.
* It encourages taking the mutable reference for a larger scope.

We now have ValueOfMut that fixes those three problems, so use that instead.

Reviewed By: bobyangyf

Differential Revision: D27938632

fbshipit-source-id: f9176980ac2158ff638ae4fda7384d1d7deca5b4

Summary: It used to be that OwnedFrozenValue was a bit of YOLO - half the methods broke the invariants, and nothing was marked unsafe. Now we've started to figure out what safe alternatives might look like, start to mark the unsafe functions as actually unsafe.

Reviewed By: bobyangyf

Differential Revision: D27963991

fbshipit-source-id: 1855cabac5883ddf269a3d691ce15d7a2b623094

Summary: This is safe because we have a lifetime to the OwnedFrozenValue, so use it.

Reviewed By: bobyangyf

Differential Revision: D27963994

fbshipit-source-id: a2dec829801c1c68d244a0f941db2429cdf42679

Summary: Previously this required a Module. A Module contains a FrozenHeap, and it's often useful to create an owned value from a module, but it's less flexible and forces people to use Module when a FrozenHeap might be all they really need.

Reviewed By: bobyangyf

Differential Revision: D27963992

fbshipit-source-id: c8e0196295527955adc6996053f79097b1fbb60f

Summary: Now there are safe functions, guide people towards those.

Reviewed By: bobyangyf

Differential Revision: D27963990

fbshipit-source-id: 794c55b364bb8ecfd3a45af6efa1217bf0520027

Summary: Previously they just had a big warning that they are likely to gain unsafe in future. Now we have safe alternatives, we're in a position to do this without impacting too much.

Reviewed By: bobyangyf

Differential Revision: D27963989

fbshipit-source-id: ef9547b83182235768aebb0ddbf210f8d5889e5c

Summary: It's an Arc, so make it cheap to clone.

Reviewed By: cjhopman

Differential Revision: D28028478

fbshipit-source-id: 9f0389e75a9ef4d1f1bd7ca7d37b419f69103947

Summary: CodeMap is itself an Arc around a File, so Arc<Arc<File>> was redundant. Remove the outer Arc.

Reviewed By: cjhopman

Differential Revision: D28028477

fbshipit-source-id: f6e8b87708dccd192e2e331cf0616d50878ebc10

Summary: The default isn't harmful, and is useful in one place.

Reviewed By: bobyangyf

Differential Revision: D28028476

fbshipit-source-id: 5c908c6d15c53857c12e7c36b30e2ae8bf0ea79d

Summary: No need to make the intermediate binding

Reviewed By: milend

Differential Revision: D28056752

fbshipit-source-id: e1303039c659f5a22254d7dc537619ec6eb46d48

Summary: Should slightly reduce reallocations

Reviewed By: blackm00n

Differential Revision: D28092672

fbshipit-source-id: 94652d437899643b493466d239a1b483155662ca

Summary: Previously we used with_capacity and were 1 short, so it had to do a realloc and complete copy just at the end.

Reviewed By: blackm00n

Differential Revision: D28092671

fbshipit-source-id: 2b75db6b8530fccc3484b2f92f2a753c85d85348

Summary: The spec required it to do so, but we didn't, which was wrong. Fix that, which allows us to follow more of the conformance tests.

Reviewed By: bobyangyf

Differential Revision: D28056751

fbshipit-source-id: 07daa6787425804d3f5ea5c57725ebd9e4df8764

Summary: This is a warning that lalrpop generates, so allow the warning to be turned on.

Reviewed By: bobyangyf

Differential Revision: D28091426

fbshipit-source-id: b6ece6f5c9cd38ce3540aeb98b37fc27cbcceab5

Summary: These are required in some circumstances, but it's a good warning to have, so disable the warning if it fires.

Reviewed By: bobyangyf

Differential Revision: D28091427

fbshipit-source-id: 702743b0b598bf0be3b88232586b93cb0a126a85

Summary: Not required at all, it seems.

Reviewed By: abrassel

Differential Revision: D28092301

fbshipit-source-id: 8af76d98d762820262d137c56686cacf93b061ba

Summary: The spec says that they should be opaque, presumably so they can be optimised to not allocate the whole list. We now do that, although because char allocates on the heap (for now) the win probably isn't as big as it would be. Ensures we can optimise in future without breaking though, and that we follow the spec.

Reviewed By: milend

Differential Revision: D28057098

fbshipit-source-id: 0747d758a910afebab83eef96a2a00ff6897dbb5

Summary:
The names ended up a bit out of whack with what they currently did. So I renamed:

* ParameterDefault => ParameterKind (it no longer just stored the default)
* names => kinds, since that was the main usage
* indices => names, since it was really a names mapping

Reviewed By: milend

Differential Revision: D28116873

fbshipit-source-id: d17ecc7299afb95a69bfc81444651b9652b56cb4

Summary:
Previously we stored the parameter names in the kinds Vec and the names Map. The only use of the value from nameswas for error messages, and you can reconstruct that from the values in Map, so do that. Saves reallocating every argument string twice.

The one behavioural change is that previously if a user defined the function with *my_args, we'd print error messages about the caller in terms of my_args, and now we go with *args. That is so rare, and the name after * is irrelevant, so this seems cleaner anyway. When using #[starlark_module] it was already impossible not to write args there.

Reviewed By: blackm00n

Differential Revision: D28116872

fbshipit-source-id: 71bc914ff635addec0d35abb01e138d8d9e83d70

Summary: We use both V and T in this module to be the ValueLike Value/FrozenValue type. That's confusing. Using V consistently.

Reviewed By: milend

Differential Revision: D28116874

fbshipit-source-id: f6468a3b809f3912540cb1eb3b8e4b5ef89244e1

Summary: Was never used.

Reviewed By: milend

Differential Revision: D28116869

fbshipit-source-id: 4ab6882d2d3ebc7b4a51b5fab5543cf89c29a5a8

Summary: They weren't used anyway.

Reviewed By: blackm00n

Differential Revision: D28116870

fbshipit-source-id: f9d0670f9ed9e88616c3f3d5b40563dbc058847a

Summary: You can mutate the name of various things after construction with the ComplexValue.export_as. Often that will make sense to mutate the ParametersSpec too.

Reviewed By: milend, blackm00n

Differential Revision: D28116868

fbshipit-source-id: 2ea2e2286d4f0e98182ef82d67f9633434c2077c

Summary: I'll need it if we want to preallocate anything. Also restrict us to only created RecordType, not FrozenRecordType (which wouldn't make much sense).

Reviewed By: blackm00n

Differential Revision: D28116871

fbshipit-source-id: 54c8d55c4e08615d87bd255f9dcd6fc6720f6393

Summary:
Previously we created all the parameters and the invoking function every time we created a record. Now we do it when we create the record type, and reuse it on each actual invocation. Profiling suggested this could be a bottleneck.

The downside is we don't have the name of the function being called at this point, so error messages are fractionally worse. But we do have the location of the call, which is probably the exact same information, so I don't feel too bad.

Reviewed By: blackm00n

Differential Revision: D28116867

fbshipit-source-id: 4ae716270b8126c3a3712cfecf588367efd31687

Summary: We were using both haphazardly. Switch to only V.

Reviewed By: milend

Differential Revision: D28117004

fbshipit-source-id: 0991984a0d01cb354b778a0fb2f99dd85e7fe860