Add functions to access existing modules in a scope #1

hawkw · 2017-12-26T21:37:21Z

While working on tower-rs/tower-grpc#9, I found that I sometimes had the need to access a module by name when it had previously been created in a given scope.

Changes

I've added the following new functions on the Scope and Module types:

module_mut optionally returns &mut Module if a scope contains a module with the given name.
module_or_add returns &mut Module if a scope contains a module with the given name, or creates it and returns a mut reference to it if it does not yet exist.
contains_module returns true if a scope contains a module with the given name.

I've also added a handful of unit tests for module_mut and module_or_add.

After experimenting with a number of ways to implement this functionality, I settled on storing the Modules defined in a Scope in an OrderMap separate from the scope's items vec. I changed Item::Module so that, rather than owning the Module, it stores the name of the module so that when the items vector is iterated over, it can access the module by looking it up in the modules map. The names of Modules are now reference counted, so that we can reuse the same string for the name stored in the Module struct itself, the key in the modules map, and the key stored in Item::Module.

Questions

I'm not convinced module_or_add is the best name for the "return a module if it exists, or else create it" method, but couldn't come up with a better one. If anyone could think of a name that better describes the function's behaviour without being overly verbose, that would be greatly appreciated. (I considered just calling it module but thought that might conflict with a hypothetical function to get a module by name non-mutably...)
Should there also be a function to optionally index a module by name immutably? I didn't see a use-case for this immediately but thought we might want something like that for completeness sake, by analogy to module_mut. If so, what should it (and the method currently named module_mut) be called --- module/module_mut? get_module/get_module_mut?

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

This reverts commit 50b7156.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

carllerche

Thanks for the PR.

I think the best option is to try to stick to simple. Lookup performance isn't really a huge factor here, so a sequential scan to find modules is a-ok I think. I have added some thoughts inline.

Re: naming:

If there is a _mut variant, there probably should be one that is immutable too. To avoid fn names that conflict with Rust keywords, a get_ prefix is probably a good idea.

So, get_module and get_module_mut it is!

carllerche · 2017-12-27T18:45:59Z

src/lib.rs

+    /// Modules are stored in a map rather than in this scope's
+    /// `items` vector so that they may be accessed by name directly
+    /// after being created.
+    modules: OrderMap<RcKey, Module>,


I would not bother with this, instead doing a scan of items to find a module. Again, lookup performance isn't really a factor at this point.

Hmm, my initial implementation did a scan through items, but I ended up going with this approach because i found it to be simpler (esp. given the borrow-checker gymnastics that ended up being necessary to make module_or_add work).

carllerche · 2017-12-27T18:46:09Z

src/lib.rs

+    /// to index the scope's map of modules. This way, modules can be looked up
+    /// by name, but also are defined at ordered locations in the `items` 
+    /// vector.
+    Module(Rc<String>),


I'd probably revert this.

carllerche · 2017-12-27T18:46:37Z

src/lib.rs

@@ -231,12 +251,20 @@ pub struct Formatter<'a> {
    dst: &'a mut String,

    /// Number of spaces to start a new line with
+    /// 


Extra line?

carllerche · 2017-12-27T18:46:44Z

src/lib.rs

@@ -57,7 +77,7 @@ enum Item {
 #[derive(Debug, Clone)]
 pub struct Module {
    /// Module name
-    name: String,
+    name: Rc<String>,


I'd revert this.

carllerche · 2017-12-27T18:48:01Z

src/lib.rs

@@ -262,19 +291,62 @@ impl Scope {
            .or_insert_with(|| Import::new(path, ty))
    }

+    /// Returns true if a module named `name` exists in this scope.
+    pub fn contains_module<Q: ?Sized>(&self, name: &Q) -> bool 


We probably don't need this function right now. I would stick with get_module and get_module_mut. These functions return Option so you can just do if scope.get_module("name").is_some().

Again, we can re-evaluate later if it ends up sucking.

carllerche · 2017-12-27T18:48:51Z

src/lib.rs

+    /// In many cases, the [`module_or_add`] function is preferrable, as it will
+    /// return the existing definition instead.
+    /// 
+    /// [`module_or_add`]: #method.module_or_add
    pub fn push_module(&mut self, item: Module) -> &mut Self {


This probably should panic if called and there already is a module with the same name in the scope.

carllerche · 2017-12-27T18:49:39Z

src/lib.rs

+
+    /// Returns a mutable reference to a module, creating it if it does 
+    /// not exist.
+    pub fn module_or_add(&mut self, name: &str) -> &mut Module {


I'd get rid of this for now. The check can be done w/ other functions.

If this becomes a pattern, then we can look at all the instances (modules, structs, etc...) and see if we can come up with a good name.

Hmm...I opened the PR with the primary goal of adding this function specifically, because I needed it in my changes to tower-grpc (get_module and get_module_mut were added only because it was more or less trivial to add them as well while making this change), and currently, it's the only one of these functions that's currently used in a consumer of codegen (my branch of tower-grpc).

While it's certainly possible to remove this & rewrite the functions that use it, I think that code is a bit clearer with this function, and my suspicion is that it's it's likely to be the most common use-case for retrieving a module...

(now that module_mut has been renamed to get_module_mut (299c69a), what about renaming this to just module, by analogy with import, which also has similar "get or else add" behavior?)

carllerche · 2017-12-27T18:49:55Z

src/lib.rs

+    }
+
+    /// Returns a mutable reference to a module if it is exists in this scope. 
+    pub fn module_mut(&mut self, name: &str) -> Option<&mut Module> {


This probably should be renamed get_module_mut to match get_module.

- Remove `contains_module` - Rename `module_mut` -> `get_module_mut` - Add `get_module` - Make `get_module`/`get_module_mut` generic over key type. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2018-01-02T22:24:03Z

@carllerche sorry I didn't comment earlier; I've made a majority of the changes you've requested. I disagree with your comment that the function formally called module_or_add should be removed and have explained my rationale; but would be willing to remove it if you still think it ought to go.

carllerche

Some thoughts left inline.

carllerche · 2018-01-05T21:19:16Z

src/lib.rs

+    /// to index the scope's map of modules. This way, modules can be looked up
+    /// by name, but also are defined at ordered locations in the `items` 
+    /// vector.
+    Module(String),


I'd rather avoid the additional indirection for now.

If doing a sequential scan in the lookup fns is problematic, we can explore replacing items: Vec<Item> with an OrderMap as order is preserved with that structure as well. This probably should happen in a separate PR.

Replacing items with an OrderMap was the second approach I tried (50b7156), it turned out to be problematic as not all items had obvious keys into that map, and it was difficult to determine keys that wouldn't result in some items being clobbered when they shouldn't be. I'll take another crack at the sequential scan instead.

Okay, done in 71348a8.

carllerche · 2018-01-05T21:21:32Z

src/lib.rs

+
+    /// Returns a mutable reference to a module, creating it if it does 
+    /// not exist.
+    pub fn get_or_add_module(&mut self, name: &str) -> &mut Module {


Could this be named get_or_new_module in this PR?

I agree the new terminology is not ideal, but that is the convention currently used for all other fns. We can approach improving the naming scheme in another PR.

Sure, changed it back in 3824853.

This reverts commit d64445d.

hawkw · 2018-01-05T22:16:00Z

@carllerche, I think I've made all the changes you requested.

carllerche

Thanks!

One last relatively minor thought.

Also, if you want to pursue the new rename, I'd recommend opening an issue w/ a proposal so we can bikeshed the naming!

carllerche · 2018-01-05T23:33:51Z

src/lib.rs

+        String: PartialEq<Q>,
+    {
+        self.items.iter_mut()
+            .find(|item| if let &mut Item::Module(ref module) = *item { 


The find(...).map(...) sequence could probably be replaced by filter_map(...).next() to avoid having to match twice.

I thought about that when implementing this originally, and then forgot. Changed in b40b1e6.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Remove travis-ci, add gitlab ci Closes #1 See merge request IovoslavIovchev/codegen!5

hawkw added 9 commits December 22, 2017 15:00

Add function to get a mut reference to an existing module

b12b2f1

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Hacky first pass on backing items with ordermap

50b7156

bring back push_x for grpc

753520b

Revert "Hacky first pass on backing items with ordermap"

f49db17

This reverts commit 50b7156.

Reimplement module_or_add but fix weird clobbering

117296b

Organise & add docs

83abaef

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Add documentation

da4826c

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Add contains_module utility method

3dd7297

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Add all of Scope's module methods to Module

8a8e035

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw mentioned this pull request Dec 26, 2017

Preliminary support for .proto files with multiple services tower-rs/tower-grpc#10

Merged

carllerche requested changes Dec 27, 2017

View reviewed changes

hawkw added 3 commits December 27, 2017 13:41

API improvement pass

299c69a

- Remove `contains_module` - Rename `module_mut` -> `get_module_mut` - Add `get_module` - Make `get_module`/`get_module_mut` generic over key type. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Review feedback

0c93c30

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Remove module name string interning and ref counting

d99c08d

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw added 2 commits January 2, 2018 15:05

mv module_or_add -> get_or_new_module

d616e01

Sorry for churn!

d64445d

carllerche requested changes Jan 5, 2018

View reviewed changes

hawkw added 2 commits January 5, 2018 13:46

Revert "Sorry for churn!"

3824853

This reverts commit d64445d.

Replace modules map with linear search

71348a8

carllerche requested changes Jan 5, 2018

View reviewed changes

hawkw and others added 2 commits January 5, 2018 15:42

Review feedback

b40b1e6

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Style fixes

9afa2cd

carllerche approved these changes Jan 6, 2018

View reviewed changes

Style fixes fixes

495ae89

carllerche merged commit 533ca26 into carllerche:master Jan 8, 2018

hawkw added a commit to tower-rs/tower-grpc that referenced this pull request Jan 8, 2018

Depend on master codegen now that carllerche/codegen#1 is merged

b947c36

hawkw added a commit to tower-rs/tower-grpc that referenced this pull request Jan 8, 2018

Depend on master codegen now that carllerche/codegen#1 is merged

78165a2

hawkw added a commit to tower-rs/tower-grpc that referenced this pull request Jan 9, 2018

Depend on master codegen now that carllerche/codegen#1 is merged

c73e21d

hawkw added a commit to tower-rs/tower-grpc that referenced this pull request Jan 9, 2018

Depend on master codegen now that carllerche/codegen#1 is merged

a42b1a6

hawkw added a commit to tower-rs/tower-grpc that referenced this pull request Jan 10, 2018

Depend on master codegen now that carllerche/codegen#1 is merged

b9c76ae

hawkw added a commit to tower-rs/tower-grpc that referenced this pull request Jan 10, 2018

Depend on master codegen now that carllerche/codegen#1 is merged

80ca166

SebastienGllmt referenced this pull request in dcSpark/codegen Jan 15, 2023

Merge branch 'gitlab-ci' into 'master'

fb85354

Remove travis-ci, add gitlab ci Closes #1 See merge request IovoslavIovchev/codegen!5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functions to access existing modules in a scope #1

Add functions to access existing modules in a scope #1

hawkw commented Dec 26, 2017

carllerche left a comment

carllerche Dec 27, 2017

hawkw Dec 27, 2017

carllerche Dec 27, 2017

carllerche Dec 27, 2017

carllerche Dec 27, 2017

carllerche Dec 27, 2017

carllerche Dec 27, 2017

carllerche Dec 27, 2017

hawkw Dec 27, 2017

hawkw Dec 27, 2017

carllerche Dec 27, 2017

hawkw commented Jan 2, 2018

carllerche left a comment

carllerche Jan 5, 2018

hawkw Jan 5, 2018

hawkw Jan 5, 2018

carllerche Jan 5, 2018

hawkw Jan 5, 2018

hawkw commented Jan 5, 2018

carllerche left a comment

carllerche Jan 5, 2018

hawkw Jan 5, 2018

Add functions to access existing modules in a scope #1

Add functions to access existing modules in a scope #1

Conversation

hawkw commented Dec 26, 2017

Changes

Questions

carllerche left a 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

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

hawkw commented Jan 2, 2018

carllerche left a 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

hawkw commented Jan 5, 2018

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment