Implement guard-safe MapSet.is_member/2

[christhekeele]

Per discussion on elixir-lang-core, this provides a guard-safe membership check for MapSets without modifying the existing MapSet.member?/2.

[josevalim]

I'd like to hear the opinion of other Elixir members before merging. :)

[sabiwara]

LGTM 💜

I thought about two possible alternatives but I think the original one is the best, just sharing them for the record:

• Kernel.is_map_set_member - avoids the need for require and mirrors is_map_key, but keeping it scoped in MapSet makes sense
• Have a Kernel.map_set/1 macro which could allow both construction and pattern-matching, e.g. map_set([1, 2, 3]) and map_set([^elem]) = ... => confusing since the fact it is partial is not obvious

[michallepicki]

It is probably not that relevant to most people, especially with the new type system coming up, but I think this probably breaks Dialyzer's "opaqueness" access rules and will cause bubbling up "this function will never return" warnings when used

[christhekeele]

@michallepicki: I think this probably breaks Dialyzer's "opaqueness" access rules and will cause bubbling up "this function will never return" warnings when used

Hm, I do indeed get this warning when using this macro copy-pasted into a project:

The guard test is_map_key(_connection@1::any(),'Elixir.MapSet':internal(_) | sets:set(_))
 breaks the opaqueness of its argument.

[sabiwara]

It is probably not that relevant to most people, especially with the new type system coming up, but I think this probably breaks Dialyzer's "opaqueness" access rules and will cause bubbling up "this function will never return" warnings when used

Good point, we could use quote generated: true do to fix this hopefully?

[michallepicki]

Good point, we could use quote generated: true do to fix this hopefully?

If this causes "bubbling up" no_return warnings to all functions calling this code (I haven't checked), then I think marking it as "generated" wouldn't help (see my proposal to change this Dialyzer behavior: erlang/otp#5503 )

edit: I tried to come up with code that causes more Dialyzer issues and failed. 🎉 So maybe when the opaqueness violation happens in pattern matching like here (and not when constructing or returning data), it doesn't affect Dialyzer that much

[christhekeele]

It looks like Dialyzer does not bubble up no_returns from this in my experimentation, but it does warn about opaqueness (and generated: true does not fix). Removing MapSet's @opaque internal() type also does not fix because it simply wraps the opaque type :sets.set(value).

[josevalim]

Just one suggestion and we can ship it :)

[christhekeele]

Suggestion accepted, and documentation of failure modes added!

[christhekeele]

Closed questions

I think this PR as-is has answered the finer points:

• Do we raise for non-mapsets?

  Yes, because MapSet.member? and is_map_key do

• What do we raise?

  ArgumentError

• How do we behave in guards?

  We still "raise" (via is_struct() or :fail)

  This can lead to suprising behaviour (see: Do not raise on non-integer values in is_odd/is_even #7063) but is internally consistent:

  • the non-guard version raises because MapSet.member? and is_map_key do
  • so the guard version does as well
  • and the well-documented if not well-known behaviour of errors in guards is to bail on the current when clause

• Do we document the failure modes?

  With the current behaviour, I don't think we need to elaborate any further, despite the footgun.

  • guard failures short-circuiting is well-documented
  • current is_member/2 docs say it behaves like (not committing to raising the same exception type as) member?/2
  • member/2 does not document its failure mode here, nor exception type, so by not defining this behaviour in documentation, we leave it open to changing without commitment

---

Neutral questions

Insofar as how we actually do the is_struct check in both guard form and normal form, I have a weak preference (is_struct in both for parity) but there have been a few other preferences presented in code review here (ex. .__struct__ == MapSet in the guard form, and pattern matching on cond in the normal form, by @josevalim).

At the end of the day, I'm happy to ship it with any variant.

---

Remaining wart

No variant I've tried seems to prevent dialyzer from warning on accessing the underlying opaque type calls, which I don't love, as part of the goal of this PR was to enable folk to do membership testing in guards without worrying about the private underlying type...

But a dialyzer warning is better than requiring folk to use the internal struct keys of MapSet for guard-safety, so I think it's worth the compromise. Dialyzer config can always prevent the warning. I'm not sure what more can be done on this front.

[christhekeele]

To clairfy the dialyzer warning I am seeing, they are only when:

• is_member/2 is used (with any struct-checking variant proposed)
• inside another defguard is_xxx
• warning at the callsite of where is_xxx is used

Other usages don't seem to warn.

I'm learning I have more to learn about @opaque types, as this was surprising to me! But I guess it makes sense, the full macro expansion places code that accesses the opaque type fully into a module other than MapSet.

[sabiwara]

LGTM 💜

[josevalim]

I am sorry folks, but I am still not confident we should merge this. For example, there is no guarantee the most efficient MapSet implementation will always have a single map at the root. Therefore, I will close this.