Fix Issue 19107 - -de produces compilation error, -dw does not

[JinShil]

Due to the error gagging, deprecations are not being displayed in typeSemantic. This PR remedies that by running typeSemantic again without gagging if it previously succeeded. It's not a very elegant solution, but I don't think there's any other way without a major DMD refactor.

See discussion in the following PRs for more context:
CyberShadow/ae#33
#8443

cc @CyberShadow

[dlang-bot]

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19107	blocker	-de produces compilation error, -dw does not

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

1. Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

2. Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8519"

[JinShil]

I think this will reveal a lot of potential problems in the D ecosystem that will need to be fixed as certain warnings and deprecations have been gagged for years.

[JinShil]

This is indirectly blocking #8443

[CyberShadow]

Have you checked if this has any impact on performance? Not a dmd expert but performing semantics twice sounds like might be expensive.

As an alternative, would it be possible to count if any warning/deprecation messages would have been emitted during gagging, and only run the semantics a second time in that case?

[JinShil]

Have you checked if this has any impact on performance? Not a dmd expert but performing semantics twice sounds like might be expensive.

It appears Type.trySemantic is only called in one place in the compiler: is expressions:

dmd/src/dmd/expressionsem.d

Line 4791 in 750f024

Type t = e.targ.trySemantic(e.loc, sc2);

, so I believe the impact would be relatively low, but I haven't measured it, and not sure if I reliably could.

As an alternative, would it be possible to count if any warning/deprecation messages would have been emitted during gagging, and only run the semantics a second time in that case?

Yeah, I think I can do that. Other reviewers, please let me now if that is preferred.

[JinShil]

With dlang/phobos#6642 merged (Thank you!), how can I cause this PR to pick up the new bits? I thought it would do it automatically. Do I need to wait, or initiate a new commit hash, or just close and re-open the PR?

[JinShil]

I just found out this same pattern, running semantic twice, is used elsewhere in the compiler:

dmd/src/dmd/typesem.d

Lines 3439 to 3458 in 750f024

	auto alias_e = resolveAliasThis(sc, e, gagError);
	if (!alias_e)
	return returnExp(null);
	auto die = new DotIdExp(e.loc, alias_e, ident);
	auto errors = gagError ? 0 : global.startGagging();
	auto exp = die.semanticY(sc, gagError);
	if (!gagError)
	{
	global.endGagging(errors);
	if (exp && exp.op == TOK.error)
	exp = null;
	}
	if (exp && gagError)
	// now that we know that the alias this leads somewhere useful,
	// go back and print deprecations/warnings that we skipped earlier due to the gag
	resolveAliasThis(sc, e, false);

[JinShil]

As an alternative, would it be possible to count if any warning/deprecation messages would have been emitted during gagging, and only run the semantics a second time in that case?

Implemented as a second commit.

[wilzbach]

how can I cause this PR to pick up the new bits? I thought it would do it automatically. Do I need to wait, or initiate a new commit hash, or just close and re-open the PR?

Only auto-tester will rebuild after another PR has been merged, all other CI require a new commit hash (some do a rebuild after close/reopen, but not all). For all of them there's a way to restart them via their Web interface, but it's typically a very painful process (especially now that we have so many).
tl;dr: ga -u && git commit --amend --no-edit && gp -f

[CyberShadow]

Only auto-tester

DAutoTest too.

ga -u && git commit --amend --no-edit && gp -f

What's ga and gp?

[wilzbach]

What's ga and gp?

Part of my local aliases. Sorry forgot that they are non-standard.
For the OT record:

alias ga='git add'
alias gp='git push'

[wilzbach]

There's global.showGaggedErrors why not use it for deprecations too?

[JinShil]

There's global.showGaggedErrors why not use it for deprecations too?

Do you mean global.params.showGaggedErrors? That's enabled for -verrors=spec, and I think is intended a different purpose. If I'm misunderstanding please clarify.

If you mean we should display deprecations when global.params.showGaggedErrors is true, I think that's the case when you compile with -verrors=spec and -de.

[wilzbach]

Do you mean global.params.showGaggedErrors? That's enabled for -verrors=spec, and I think is intended a different purpose. If I'm misunderstanding please clarify.

Sorry, I was trying to suggest instead of counting for the warnings (and then re-running the semantic), you could immediately print them directly if a respective global flag like global.params. showGaggedDeprecations is set?

(It assumed that was what showGaggedErrors was intended to do. Sorry)

[CyberShadow]

I think that would mean that the user will see warnings/deprecations from speculative attempts at resolving semantics that are expected to fail.

[wilzbach]

Yes I understood that, but I was talking about the fact that there's only one place where gaggedWarnings is checked, it's known before the startGagging call a few lines above, that the deprecations should be printed which is why @JinShil currently reruns typeSemantic without gagging.
So running startGagging once with a special flag should avoid the need to run it twice in this scenario, no?

[JinShil]

I don't think that will work because you only want to show deprecation messages if the entire typeSemantic call succeeds. You won't know that until it's finished.

[JinShil]

@CyberShadow What's up with DAutoTest? Is it a dub failure?

[CyberShadow]

Definitely a Dub failure, though never saw it print an error in that format before. @wilzbach Do you know?

No versions for ddoc_preprocessor for registry at https://code.dlang.org/ (fallback ["registry at http://code.dlang.org/", "registry at https://code-mirror.dlang.io/", "registry at https://code-mirror2.dlang.io/", "registry at https://dub-registry.herokuapp.com/"])

It looks like it tried all those mirrors already? Also I thought treating network failures as "No versions" was due to be fixed. Or was it not a network failure?

[wilzbach]

ddoc_preprocessor is a local path dependency (like dmd), so it can't even be downloaded from the registry (no idea why dub tries anyhow), but it looks more like a problem with the version resolver:

No versions for ddoc_preprocessor for registry at https://code.dlang.org/ (fallback ["registry at http://code.dlang.org/", "registry at https://code-mirror.dlang.io/", "registry at https://code-mirror2.dlang.io/", "registry at https://dub-registry.herokuapp.com/"])
Return for ddoc_preprocessor: [~master]
Search for versions of dmd (1 package suppliers)
Return for dmd: [~master, ~stable, ~newCTFE, ~markdown, ~dcache]
The dependency resolution process is taking too long. The dependency graph is likely hitting a pathological case in the resolution algorithm. Please file a bug report at https://github.com/dlang/dub/issues and mention the package recipe that reproduces this error.
posix.mak:762: recipe for target 'dpl-docs' failed

Not sure why it spuriously appears, but maybe dlang/dub#1513 helps at least in the future.

[JinShil]

@CyberShadow I've tried re-running DAutoTest several times, and it just won't pass. Other PRs I've submitted pass fine. Could you please dig a little bit and see if you can find anything?

[CyberShadow]

The master base branch build has failed too:

The dependency resolution process is taking too long. The dependency graph is likely hitting a pathological case in the resolution algorithm. Please file a bug report at https://github.com/dlang/dub/issues and mention the package recipe that reproduces this error.

Actually, the stable branch has been failing to build for a while now:

http://dtest.dlang.io/history/stable

@wilzbach Any idea if anything between that success/failure could have introduced a regression?

[wilzbach]

@wilzbach Any idea if anything between that success/failure could have introduced a regression?

No. Sorry, we haven't done much at dlang.org (https://github.com/dlang/dlang.org/commits/master) in the last weeks.

However, I agree that this is unrelated - I have seen this error way too often over the last week :/
Maybe we can get away with not running dub upgrade --missing-only --root=dpl-docs before we run dub build?

[wilzbach]

FYI: this will also help #7241 to move forward.

[RazvanN7]

I just found out this same pattern, running semantic twice, is used elsewhere in the compiler:

On the long run we should probably use a more elegant solution, like having a queue of gagged errors/deprecations/warnings and just print them without having to copy parts of the AST and perform semantic twice. This would negligibly increase the memory usage, but it will increase performance.

[wilzbach]

A naive attempt at fixing our problem by bumping the dub version used at dlang.org: dlang/dlang.org#2432

[wilzbach]

Seems like DAutoTest is entirely broken for stable, but as this should go into 2.082 and not 2.081.2 anyhow, I rebased this, s.t. we can move forward with this.
Reasoning for introducing this in 2.082: we shouldn't deprecate things in patch updates (that's pretty much against SemVer) and a user only expects stability patches/improvements/fixes when they upgrade a patch version.