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

Be conservative with visibility #2523

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Be conservative with visibility #2523

wants to merge 4 commits into from

Conversation

bakaq
Copy link
Contributor

@bakaq bakaq commented Sep 3, 2024

Part of #2490. Related previous discussion #2512. This PR does the following:

  • Refactors all of the code of the binary into a run_binary() function in the library interface, so that we don't have to make stuff like Atom and atom! public.
  • Turns everything that didn't make sense to be pub (and wasn't used in tests or benchmarks) into pub(crate) (or stricter) instead. Maybe most of those could/should be even stricter, but this is out of scope because it would not be a breaking change if done after this PR.

Currently the following are public (our semver surface) and pub used at the top level of the library:

  • Structs
    • Machine
      • All fields private.
      • new_lib
      • load_module_string
      • consult_module_string
      • run_query
      • run_query_iter
      • with_test_streams (only used in integration tests)
      • test_load_file (only used in integration tests)
      • test_load_string (only used in Wasm build)
      • get_inference_count (only used in benchmarks)
      • new
    • MachineConfig
      • All fields public, not #[non_exhaustive].
      • in_memory
      • with_toplevel
    • QueryMatch
      • All fields public, not #[non_exhaustive].
    • QueryState
      • All fields private.
  • Enums
    • QueryResolution
      • Not #[non_exhaustive]
    • QueryResolutionLine
      • Not #[non_exhaustive]
    • StreamConfig
      • Not #[non_exhaustive]
    • Value
      • Not #[non_exhaustive]
  • Functions
    • run_binary
  • Type Aliases
    • QueryResult

cargo check goes crazy with the amount of dead code warnings. It didn't point it before because pub code isn't considered in dead code analysis. Clippy goes even crazier, it seems to be linting the generated files now for some reason (which is good, but why didn't it do that before?). No errors in either though.

Open questions

Can be addressed in another PR.

  • Should we keep the Machine methods that are only used in tests, benchmarks and the Wasm build or should we refactor to use the rest of the interface instead so that we get a cleaner public interface?
  • Should run_binary be behind a feature?

Remaining work

  • Handle visibility for the 54 #[macro_use] macros. I think all of them should probably be private, as they leak internal details and aren't really useful to users. This will be quite a chore, because Scryer Prolog seems to rely a lot on these macros being public and global. Was actually very easy.
  • Maybe deal with at least part of the dead code pointed by cargo check (maybe by just #[allow(dead_code)]ing most of it).
  • Maybe deal with Clippy warnings.

Out of scope for this PR

  • Separating binary and/or lib into separate packages in a workspace.
  • Naming.
  • Future proofing APIs (things like keeping struct fields private and #[non_exhaustive]ing enums).
  • Documentation.
  • Freezing signatures and traits.

@bakaq bakaq marked this pull request as draft September 3, 2024 03:58
@bakaq
Copy link
Contributor Author

bakaq commented Sep 3, 2024

I also want to ask @mthom if removing all of the #[macro_use]s would be too disruptive. I think everything else here is pretty harmless and orthogonal to other changes, but changing the visibility of macros will impact the whole crate and may create a lot of conflicts with other stuff being developed. (I may be overstating how much this would impact)

@bakaq
Copy link
Contributor Author

bakaq commented Sep 6, 2024

OOOHH, I was confusing how things work. The issue is actually with #[macro_export], not #[macro_use]. I think this will be significantly easier than I thought it would be.

@bakaq
Copy link
Contributor Author

bakaq commented Sep 6, 2024

Ok, I feel a bit stupid now by thinking that would be very hard. I just left all macros global (that's what #[macro_use] does), but not public (which is what #[macro_export] does). I think this is not disruptive at all to the rest of the code base.

Ok, now we already have a pretty small public interface that we can polish and document extensively:

image

Now we have to decide what to about all the warnings (39 warnings, mostly about unused code).

@bakaq
Copy link
Contributor Author

bakaq commented Sep 7, 2024

I fixed the warnings (and Clippy). I only deleted parts of code that really seemed useless or would be very easy to reimplement if we need in the future (which was most of the warnings actually). I think this is ready for review, the open questions can be decided in another PR.

@bakaq bakaq marked this pull request as ready for review September 7, 2024 00:38
@mthom
Copy link
Owner

mthom commented Sep 15, 2024

- Should we keep the Machine methods that are only used in tests, benchmarks and the Wasm build or should we refactor to use the rest of the interface instead so that we get a cleaner public interface?
- Should run_binary be behind a feature?

I don't have good answers to either of these questions I'm afraid. Most of the time it's a matter of wait and see. Looks good to me, unless there are objections or further requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants