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

Restrict global namespace pollution #94

Merged
merged 5 commits into from
Oct 9, 2019
Merged

Restrict global namespace pollution #94

merged 5 commits into from
Oct 9, 2019

Conversation

zauguin
Copy link
Member

@zauguin zauguin commented Sep 2, 2019

Currently we are creating lots of global variables. E. g. in dev we currently define the following globals:

Category 1: (official interface)

luaotfload

Category 2: (inofficial interface)

fonts
nodes

Category 3: (fontloader internal or luaotfload-fontloader interface)

callbacks
containers
caches
arguments
storage
statistics
resolvers
characters
directives
attributes
experiments

Category 4: (fontloader, should probably be local)

readarray

Category 5: (luaotfload, should be local)

logreport
current_features
splitcomma
stdout

Special (ignore for now, will be handled in the bidi-dev branch)

domultiscript

I sorted them into categories based on who causes them and how easy it is to get rid of them. Category 1, luaotfload, is ok: It is our "official" interface to the outside world. Category 2 are globals created by the fontloader which are used by third party packages and became a less official part of out interface. We should keep them to avoid breaking things.

So let's move on to the more interesting categories. Category 3 are globals created by the fontloader which are basically useless to the outside world. They occupy very nice names which could be used for other stuff without providing much value. (Most of them only provide dummy functions to avoid errors when the fontloader tries to call ConTeXt handlers) I would like to get rid of these.

Category 4 and Category 5 are "accidental globals": They are variables for which I do not think that there is any reason why they should be global in the first place, they are created by typos or missing locals. readarray is created by the fontloader, so is should either become local in ConTeXt or it can be handled together wit Category 3.

This PR consists of two steps, implemented in two commits:

  1. Handle Category 5. This shouldn't break anything, but it should still be tested.
  2. The second commit moves the fontloader into a separate environment such that categories 3 and 4 are no longer global. To avoid restricting access to any parts of the fontloader which might still be useful, they are made accessible though luaotfload.fontloader.

Before this gets merged, we should do some additional tests especially for the second step: Is there any external code which relies on these globals? (I couldn't find such code, but searching for terms like experiments or statistics in CTAN leads to lots of false positives, so I might have missed something. Also not everything is in the CTAN...)
If we find such code: Is it an important use-case? Then we could move the affeted globals to category 2 and keep them. Otherwise can the external code be changed to use luaotfload.fontloader?

The other question is if we even want such a change. It would make valuable names available and ensure better isolation of the fontloader, thereby reducing the risk that third-party code which accidentally uses the same names affects the fontloader, but on the other hand it risks breaking compatibility if some users relied on eccentric interfaces of the fontloader.

@u-fischer
Copy link
Member

We discussed the polution of the global namespace in the team some time ago and it was common consens that it makes sense to avoid it and remove unwanted entries.

Imho the most important case is luatex-ja to check - there are no good testfiles in our repo, so it is easy to overlook something here. And probably one will have to search tex.sx if some of the tables have been used - but imho as long as there are still available it should be ok.

It would be good to first merge the buildchange-stuff, to remove unneeded/merged branches (mirror-dev, mirror?) and to ensure that all branches have the same build lua/travis.yml (modulo some version numbers) and to set the version number to 3.0005 or whatever (7.0002 as in the buildchange-dev looks a bit high). Currently part of the branches install still in the luaotfload-folder and this is a bit confusing.

@zauguin zauguin merged commit fc68818 into dev Oct 9, 2019
@zauguin zauguin deleted the lessglobals-dev branch October 9, 2019 23:33
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.

None yet

2 participants